[Wallet] move "load wallet phase" to CWallet #7577

jonasschnelli commented at 11:09 AM on February 22, 2016: contributor

Another simple and easy-to-review wallet/init.cpp refactoring

jonasschnelli added the label Refactoring on Feb 22, 2016

jonasschnelli added the label Wallet on Feb 22, 2016

kirkalx commented at 8:50 PM on February 22, 2016: contributor

Looks good to me... utACK.

kirkalx commented at 3:40 AM on February 24, 2016: contributor

Does this need a bump to get travis to check it @jonasschnelli ?

jonasschnelli force-pushed on Feb 24, 2016

jonasschnelli commented at 6:49 AM on February 24, 2016: contributor

Force pushed (no changes), this should give Travis a kick.

laanwj commented at 3:39 PM on March 1, 2016: member

Concept ACK, this is a nice step toward factoring out the wallet code (as well as a step towards being able to load multiple wallets).

I do think the return condition is a bit messy, e.g. using both a NULL return value as well as a non-empty errorString as an error condition:

 +        if (!pwalletMain)
 +            return false;
...
 +        if (!errorString.empty())
 +            return InitError(errorString);

Ideally, InitLoadWallet would always return NULL if an error happened, and always fill errorString. I understand that this is a change with more impact, though: e.g. make walletInstance a scoped pointer, which is released (and then returned) only on success.

kirkalx commented at 9:24 PM on March 1, 2016: contributor

From the initial work I've done on updating some of Jonas's other wallet patches, this is familiar and I agree it can be confusing, especially in some of the more complicated changes where wallet code calls some other wallet code with its own error handling. The intention is to end up with at least one (and preferably only one) error message in the log if something goes wrong. Worst case scenario if we screw up is that the program just shuts down without an error being reported.

jonasschnelli commented at 6:33 PM on March 5, 2016: contributor

Agree with @laanwj. I wanted to keep the changes at a minimum because this is not the first try to get this into master.

Will adapt a better return code handling.

kirkalx commented at 11:02 PM on March 5, 2016: contributor

Sweet, utACK 32ffefc12ba5a5956d64a7c207f0070a11041339. Sorry, no dev machine at present so can't test...

jonasschnelli force-pushed on Mar 6, 2016

laanwj commented at 8:32 PM on March 6, 2016: member

Agree with @laanwj. I wanted to keep the changes at a minimum because this is not the first try to get this into master.

Yes, changing this may be too risky right now, at least this is straightforward and easy to review.

utACK

laanwj commented at 7:41 AM on March 11, 2016: member

Needs rebase after #7576

[Wallet] move "load wallet phase" to CWallet fc7c60d699

jonasschnelli force-pushed on Mar 11, 2016

jonasschnelli commented at 1:41 PM on March 11, 2016: contributor

Rebased.

in src/wallet/wallet.cpp:None in 4841d7d9ac outdated

3018 | +    CWallet *walletInstance = new CWallet(strWalletFile);
3019 | +    DBErrors nLoadWalletRet = walletInstance->LoadWallet(fFirstRun);
3020 | +    if (nLoadWalletRet != DB_LOAD_OK)
3021 | +    {
3022 | +        if (nLoadWalletRet == DB_CORRUPT)
3023 | +            errorString += _("Error loading wallet.dat: Wallet corrupted") + "\n";

MarcoFalke commented at 5:32 PM on March 13, 2016:

Nit: Please don't mix concatenations (+=)and assignments (=). You refactored this such that there can only be one error in the errorString, so an assignment would be sufficient.

jonasschnelli commented at 8:29 AM on March 14, 2016:

My main focus was it to make the change as "moveonly" as possible. The current code also concats error strings (https://github.com/bitcoin/bitcoin/pull/7577/files#diff-c865a8939105e6350a50af02766291b7L1454).

IMO best strategy is to move the wallet code away from init.cpp, then optimize it. This changes was already included in two closed PR (which where closed due to inactivity and lack of review). We probably should distinct between refactor/"moveonly-ish" PRs and behavior change PRs.

laanwj commented at 9:43 AM on March 14, 2016:

It is ugly but was already the case in the previous version. This literally moves the code. Let's leave improvements here to later pulls.

in src/wallet/wallet.cpp:None in 4841d7d9ac outdated

3075 | +        }
3076 | +
3077 | +        walletInstance->SetBestChain(chainActive.GetLocator());
3078 | +    }
3079 | +
3080 | +    LogPrintf("%s", errorString);

MarcoFalke commented at 5:59 PM on March 13, 2016:

You'd have to move this above the return NULL. (or remove it, as it is dead code)

jonasschnelli commented at 8:34 AM on March 14, 2016:

Right. This is dead code now. Move the log print to the init.cpp error handling (L1434).

MarcoFalke commented at 11:12 AM on March 14, 2016:

Thx for the quick fix. Good this is merged now.

MarcoFalke commented at 6:09 PM on March 13, 2016: member

Concept ACK 4841d7d

jonasschnelli force-pushed on Mar 14, 2016

[Wallet] optimize return value of InitLoadWallet() 15e6e13624

jonasschnelli force-pushed on Mar 14, 2016

laanwj merged this on Mar 14, 2016

laanwj closed this on Mar 14, 2016

laanwj referenced this in commit 5b3b5a7d71 on Mar 14, 2016

codablock referenced this in commit b244e506ec on Sep 16, 2017

codablock referenced this in commit 2748e82181 on Sep 19, 2017

codablock referenced this in commit 52c11536d1 on Dec 9, 2017

codablock referenced this in commit b3525b2e67 on Dec 19, 2017

zkbot referenced this in commit fa341bcff0 on Dec 18, 2019

random-zebra referenced this in commit e89e20eca1 on Aug 25, 2020

MarcoFalke locked this on Sep 8, 2021