Page MenuHomePhabricator

Move shared qt/bitcoind initialization steps to shared functions.
AbandonedPublic

Authored by schancel on Nov 25 2018, 23:24.

Details

Reviewers
jasonbcox
Group Reviewers
Restricted Project
Summary

Move shared initialization steps to their own functions. We eventually would like as
shared an initialization as possible so we can ensure that we can de-globalize
items such as the mempool.

Test Plan
make check

Diff Detail

Repository
rABC Bitcoin ABC
Branch
move-shared-init
Lint
Lint Passed
Unit
No Test Coverage
Build Status
Buildable 4132
Build 6335: Bitcoin ABC Buildbot (legacy)
Build 6334: arc lint + arc unit

Event Timeline

Fabien added inline comments.
src/bitcoind.cpp
178 ↗(On Diff #6098)

I don't know what the reason is for this comment, but the changes break this assumption.
Did you look at the impact of moving the data dir lock before deamonization ?

schancel added inline comments.
src/bitcoind.cpp
178 ↗(On Diff #6098)

I don't see how this matters at all. deamonization does a double fork, and will inherit the lock.

Fix locking behavior when daemonizing

src/bitcoind.cpp
169 ↗(On Diff #6107)

You may leave this block unchanged, I see no value it these changes. I you want to fix daemonization => daemonizing, please also refactor the other comments in order to have them consistent.

src/init.cpp
1763 ↗(On Diff #6107)

This should also be removed ?

schancel marked 2 inline comments as done.

Fix comment

jasonbcox requested changes to this revision.Nov 26 2018, 21:25
jasonbcox added a subscriber: jasonbcox.

I have a number of backports in my pipeline that touch this code. Please hold this diff until those are completed.

This revision now requires changes to proceed.Nov 26 2018, 21:25
schancel requested review of this revision.Mar 9 2019, 21:26

@jasonbcox Are your backports done? If not, this is a very small change to rebase onto.

Unless there is a larger plan here, this is just moving away from core without any clear benefit.

jasonbcox requested changes to this revision.Mar 20 2019, 01:33

This directly conflicts with T417. I don't think this change is worth it.

This revision now requires changes to proceed.Mar 20 2019, 01:33