Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Mar 14, 2016
Merged

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

merged 2 commits into from
Mar 14, 2016
+172 −141

Conversation

jonasschnelli
Copy link
Contributor

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

@kirkalx
Copy link
Contributor

kirkalx commented Feb 22, 2016

Looks good to me... utACK.

@kirkalx
Copy link
Contributor

kirkalx commented Feb 24, 2016

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

@jonasschnelli
Copy link
Contributor Author

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

@jonasschnelli jonasschnelli mentioned this pull request Feb 26, 2016
Merged
@laanwj
Copy link
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
Copy link
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
Copy link
Contributor Author

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
Copy link
Contributor

kirkalx commented Mar 5, 2016

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

@laanwj
Copy link
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
Copy link
Member

laanwj commented Mar 11, 2016

Needs rebase after #7576

@jonasschnelli
Copy link
Contributor Author

Rebased.

maflcko
maflcko reviewed Mar 13, 2016
View reviewed changes
src/wallet/wallet.cpp
if (nLoadWalletRet != DB_LOAD_OK)
{
if (nLoadWalletRet == DB_CORRUPT)
errorString += _("Error loading wallet.dat: Wallet corrupted") + "\n";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@maflcko
Copy link
Member

maflcko commented Mar 13, 2016

Concept ACK 4841d7d

@laanwj laanwj merged commit 15e6e13 into bitcoin:master Mar 14, 2016
laanwj added a commit that referenced this pull request Mar 14, 2016
@laanwj
Merge #7577: [Wallet] move "load wallet phase" to CWallet
5b3b5a7 
15e6e13 [Wallet] optimize return value of InitLoadWallet() (Jonas Schnelli)
fc7c60d [Wallet] move "load wallet phase" to CWallet (Jonas Schnelli)
codablock pushed a commit to codablock/dash that referenced this pull request Dec 19, 2017
@laanwj @codablock
Merge bitcoin#7577: [Wallet] move "load wallet phase" to CWallet
b3525b2 
15e6e13 [Wallet] optimize return value of InitLoadWallet() (Jonas Schnelli)
fc7c60d [Wallet] move "load wallet phase" to CWallet (Jonas Schnelli)
@str4d str4d mentioned this pull request Dec 18, 2019
Merged
zkbot added a commit to zcash/zcash that referenced this pull request Dec 18, 2019
@zkbot
Auto merge of #4270 - str4d:2074-wallet-2, r=str4d
fa341bc 
Bitcoin wallet PRs 2

Cherry-picked from the following upstream PRs:

- bitcoin/bitcoin#7576
- bitcoin/bitcoin#7577
- bitcoin/bitcoin#7608
- bitcoin/bitcoin#7691
- bitcoin/bitcoin#7905
@random-zebra random-zebra mentioned this pull request Aug 17, 2020
Merged
random-zebra added a commit to PIVX-Project/PIVX that referenced this pull request Aug 25, 2020
@random-zebra
Merge #1811: [Wallet][Refactoring] wallet/init refactoring backports
e89e20e 
7e71759 Wallet: Split main logic from InitLoadWallet into CreateWalletFromFile (random-zebra)
e585dad [Wallet] refactor wallet/init interaction (random-zebra)
49646b2 [Refactor] Nuke zwalletMain global object (random-zebra)
f5f9df9 Wallet: Let the interval-flushing thread figure out the filename (Luke Dashjr)
74445e4 [wallet] Move hardcoded file name out of log messages (random-zebra)
3f1838d [Wallet] optimize return value of InitLoadWallet() (random-zebra)
539dea4 [Wallet] move "load wallet phase" to CWallet (random-zebra)
7644318 [Wallet] move wallet help string creation to CWallet (random-zebra)
a9d69b8 [trivial] Reuse translation and cleanup DEFAULT_* values (random-zebra)
cfd007a [Bug] Omit wallet-related options from -help when wallet not supported (random-zebra)
da642e6 [Refactor] More constant default values cleanup (random-zebra)
a45275a [Refactor] Implement CBaseChainParams::BaseParams(Network) (random-zebra)
09abb98 Constrain constant values to a single location in code (random-zebra)

Pull request description:

  Adapts the following refactoring PRs:

  - bitcoin#6961 Constrain constant values to a single location in code
  - bitcoin#7576 [Wallet] move wallet help string creation to CWallet
  - bitcoin#7577 move "load wallet phase" to CWallet
  - bitcoin#7608 Move hardcoded file name out of log messages
  - bitcoin#7691 refactor wallet/init interaction
  - bitcoin#8776 Wallet refactoring leading up to multiwallet

ACKs for top commit:
  furszy:
    re ACK 7e71759
  Fuzzbawls:
    ACK 7e71759

Tree-SHA512: 5fad328b9ddf8187af97d3d5fb285d0b67e73d51ac1bc44a3d57d0af86bce8b34efaab539b7bdbc4f9a2fa575a936f83788cffcc9c6d6d04cd3e63b19d399400
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
Refactoring Wallet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jonasschnelli @kirkalx @laanwj @maflcko