refactor and optimize init.cpp/CWallet interaction #5990
Conversation
jonasschnelli
force-pushed
the
2015/04/wallet_init_decoup
branch
from
d018f78 to
4e5c56e
Compare
src/init.cpp
Outdated
There was a problem hiding this comment.
Would a !pwalletMain help with a NULL pointer here?
There was a problem hiding this comment.
It's within a if (CWallet::IsDisabled()) else control structure. pwalletMain can't be NULL there. But i assume it won't hurt adding a !pwalletMain
jonasschnelli
changed the title
|
ditto previous PR - code movement in separate commit concept ACK |
|
Here the moved code did need some adaption. I was trying to make more smaller commits. This was not possible with the fact that every commit should also passes travis (and result in a proper compilable bitcoind/qt) |
|
fair enough |
jonasschnelli
force-pushed
the
2015/04/wallet_init_decoup
branch
from
cc36df8 to
c0b1825
Compare
|
Rebased. I'm not sure if i should include another commit which would then completely decouple the wallet over signaling to allow multiple wallet devices: With this commit it would be a useful groundwork for ZMQ and a wallet rewriting without breaking the current wallet. |
jonasschnelli
force-pushed
the
2015/04/wallet_init_decoup
branch
from
c0b1825 to
ddae534
Compare
jonasschnelli
force-pushed
the
2015/04/wallet_init_decoup
branch
from
ddae534 to
6bf1347
Compare
|
Rebased. |
jonasschnelli
force-pushed
the
2015/04/wallet_init_decoup
branch
from
2f1adc6 to
72ba404
Compare
|
Added another commit on top. This would decouple the wallet from the core and would allow to have multiple wallets (validation devices) over signaling. Together with #5994 it would basically remove (almost) all wallet #ifdefs. The goal is to just have one wallet #ifdef at the point where we register for signals. IMO this is a massiv step forward of core/wallet separation. |
jonasschnelli
force-pushed
the
2015/04/wallet_init_decoup
branch
from
72ba404 to
1588cf2
Compare
Reduced wallet/core coupling. - hides CWalletDB behind CWallet - reduces ENABLE_WALLET ifdefs
jonasschnelli
force-pushed
the
2015/04/wallet_init_decoup
branch
from
1588cf2 to
fb5b746
Compare
|
Rebased |
There was a problem hiding this comment.
Why are we using .count here and not e.g. GetArg()?
There was a problem hiding this comment.
This code has been moved 1:1 (better review capabilities).
|
Travis issue seems to be non-related to this PR (race condition) |
There was a problem hiding this comment.
Not that it would matter that much, but how about using a static variable, that would allow to use GetWalletFile() without the need to store it into another std::string when using the function, which happens quite a few times.
jonasschnelli
force-pushed
the
2015/04/wallet_init_decoup
branch
2 times, most recently
from
6b6d677 to
fb5b746
Compare
|
Rebased this PR. |
jonasschnelli
force-pushed
the
2015/04/wallet_init_decoup
branch
5 times, most recently
from
0ca5c01 to
fb5b746
Compare
|
ut ACK - this seems a reasonable refactor to merge soonish even considering my comments on bitcoin-dev about refactoring - @jonasschnelli seems like the main person working on improving CoreWallet right now, so downstream developer breakage seems unlikely. |
|
This would be a major step in decoupling the wallet from the core (codebase and not process decoupling). I'm ready to continue, but would like to get some concept ACK because this PR is itself relatively "heavy" and requires rebases all the time because it moves code away from |
There was a problem hiding this comment.
NIT: Add a newline before this and before the next step?
|
concept ACK, will review some point soon |
|
light utACK, will review again after rebase @jonasschnelli |
|
Needs rebase. |
|
Is this pull still relevant with recent wallet/init.cpp changes? If so needs rebase, otherwise let's close |
|
Right. Closing. #7691 (merged) did contain most of this changes (a slightly different concept though). |
6 participants
Reduced wallet/core coupling.
This would also slowly lay groundwork for a possible new wallet candidate.