New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from Mar 14, 2016

Conversation

Projects
None yet
4 participants
@jonasschnelli
Member

jonasschnelli commented Feb 22, 2016

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

@kirkalx

This comment has been minimized.

Show comment
Hide comment
@kirkalx

kirkalx Feb 22, 2016

Contributor

Looks good to me... utACK.

Contributor

kirkalx commented Feb 22, 2016

Looks good to me... utACK.

@kirkalx

This comment has been minimized.

Show comment
Hide comment
@kirkalx

kirkalx Feb 24, 2016

Contributor

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

Contributor

kirkalx commented Feb 24, 2016

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

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Feb 24, 2016

Member

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

Member

jonasschnelli commented Feb 24, 2016

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 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.

Member

laanwj commented Mar 1, 2016

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

This comment has been minimized.

Show comment
Hide comment
@kirkalx

kirkalx Mar 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.

Contributor

kirkalx commented Mar 1, 2016

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 5, 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.

Will adapt a better return code handling.

Member

jonasschnelli commented Mar 5, 2016

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

This comment has been minimized.

Show comment
Hide comment
@kirkalx

kirkalx Mar 5, 2016

Contributor

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

Contributor

kirkalx commented Mar 5, 2016

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 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

Member

laanwj commented Mar 6, 2016

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Mar 11, 2016

Member

Needs rebase after #7576

Member

laanwj commented Mar 11, 2016

Needs rebase after #7576

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Mar 11, 2016

Member

Rebased.

Member

jonasschnelli commented Mar 11, 2016

Rebased.

if (nLoadWalletRet != DB_LOAD_OK)
{
if (nLoadWalletRet == DB_CORRUPT)
errorString += _("Error loading wallet.dat: Wallet corrupted") + "\n";

This comment has been minimized.

@MarcoFalke

MarcoFalke Mar 13, 2016

Member

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.

@MarcoFalke

MarcoFalke Mar 13, 2016

Member

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.

This comment has been minimized.

@jonasschnelli

jonasschnelli Mar 14, 2016

Member

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.

@jonasschnelli

jonasschnelli Mar 14, 2016

Member

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.

This comment has been minimized.

@laanwj

laanwj Mar 14, 2016

Member

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.

@laanwj

laanwj Mar 14, 2016

Member

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.

@MarcoFalke

View changes

Show outdated Hide outdated src/wallet/wallet.cpp
@MarcoFalke

This comment has been minimized.

Show comment
Hide comment
@MarcoFalke

MarcoFalke Mar 13, 2016

Member

Concept ACK 4841d7d

Member

MarcoFalke commented Mar 13, 2016

Concept ACK 4841d7d

@laanwj laanwj merged commit 15e6e13 into bitcoin:master Mar 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Mar 14, 2016

Merge #7577: [Wallet] move "load wallet phase" to CWallet
15e6e13 [Wallet] optimize return value of InitLoadWallet() (Jonas Schnelli)
fc7c60d [Wallet] move "load wallet phase" to CWallet (Jonas Schnelli)

codablock added a commit to codablock/dash that referenced this pull request Sep 16, 2017

Merge #7577: [Wallet] move "load wallet phase" to CWallet
15e6e13 [Wallet] optimize return value of InitLoadWallet() (Jonas Schnelli)
fc7c60d [Wallet] move "load wallet phase" to CWallet (Jonas Schnelli)

codablock added a commit to codablock/dash that referenced this pull request Sep 19, 2017

Merge #7577: [Wallet] move "load wallet phase" to CWallet
15e6e13 [Wallet] optimize return value of InitLoadWallet() (Jonas Schnelli)
fc7c60d [Wallet] move "load wallet phase" to CWallet (Jonas Schnelli)

codablock added a commit to codablock/dash that referenced this pull request Dec 9, 2017

Merge #7577: [Wallet] move "load wallet phase" to CWallet
15e6e13 [Wallet] optimize return value of InitLoadWallet() (Jonas Schnelli)
fc7c60d [Wallet] move "load wallet phase" to CWallet (Jonas Schnelli)

codablock added a commit to codablock/dash that referenced this pull request Dec 19, 2017

Merge #7577: [Wallet] move "load wallet phase" to CWallet
15e6e13 [Wallet] optimize return value of InitLoadWallet() (Jonas Schnelli)
fc7c60d [Wallet] move "load wallet phase" to CWallet (Jonas Schnelli)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment