Skip to content
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

multiprocess: Start using init makeNode, makeChain, etc methods #22219

Merged
merged 1 commit into from
Sep 16, 2021

Conversation

ryanofsky
Copy link
Contributor

Use interfaces::Init::make* methods instead of interfaces::Make* functions, so interfaces can be constructed differently in different executable without having to change any code. (So for example bitcoin-gui can make an interfaces::Node pointer that communicates with a bitcoin-node subprocess, while bitcoin-qt can make an interfaces::Node pointer that controls node code in the same process.)


This PR is part of the process separation project. The commit was first part of larger PR #10102.

@maflcko
Copy link
Member

maflcko commented Jun 11, 2021

ci fails. Maybe this needs rebase on #22216 ?

@ryanofsky
Copy link
Contributor Author

Rebased 12ca448 -> 40dad10 (pr/ipc-make.1 -> pr/ipc-make.2, compare) after #22216 to fix implicit dependency on #22216 causing CI failures https://github.com/bitcoin/bitcoin/pull/22219/checks?check_run_id=2798033917

@ryanofsky
Copy link
Contributor Author

ci fails. Maybe this needs rebase on #22216 ?

Thanks! Rebased now. Nice catch of this dependency

@maflcko
Copy link
Member

maflcko commented Jun 11, 2021

windows build is red 😬

@ryanofsky
Copy link
Contributor Author

ryanofsky commented Jun 11, 2021

windows build is red 😬

Hopefully fixed!


Updated 40dad10 -> 7a879af (pr/ipc-make.2 -> pr/ipc-make.3, compare) to fix appveyor build error https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/39561119#L32
Updated 7a879af -> 8da084a (pr/ipc-make.3 -> pr/ipc-make.4, compare) to to fix appveyor build error https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/39567970#L33
Rebased 8da084a -> c50facd (pr/ipc-make.4 -> pr/ipc-make.5, compare) after bitcoin-core/gui#361 to revert it because that change is no longer necessary with other changes in this PR

Copy link
Member

@jamesob jamesob left a comment

Choose a reason for hiding this comment

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

ACK c50facd (jamesob/ackr/22219.1.ryanofsky.multiprocess_start_using)

Built and ran unittests locally. Changes look fine and mostly boil down to naming and some pointer/ref swaps.

Show signature data

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

ACK c50facd030ec5bd2c7fc2b55486a3cb77b59b1d8 ([`jamesob/ackr/22219.1.ryanofsky.multiprocess_start_using`](https://github.com/jamesob/bitcoin/tree/ackr/22219.1.ryanofsky.multiprocess_start_using))

Built and ran unittests locally. Changes look fine and mostly boil down to naming and some pointer/ref swaps.
-----BEGIN PGP SIGNATURE-----

iQIzBAEBCgAdFiEEGNRVI1NPYuZCSIrGepNdrbLETwUFAmDJEKYACgkQepNdrbLE
TwVgRA//UJL92+4xQvu/zOKP4Rv5BnnAx2Tr9g2Zhcxj9dTisiJfXaxBnTrW0Wcx
BaHuMZT0oq4aCpoi/rs686a2hR56Kts2SxBv9wKfOmQ5UobuMfgGGrcrr3XYPivG
YK+mt7RBc0yRhEUFv+dwvk+dGW9/lCAy3YukvfEGLSu57YGM8Aj/8k69cqOZGkTT
XQl1b45Aguk2EWBC56Vp916+qbU1kkGb1Y1Fpph0Ac7IOUeqHaOcBOlsnHIomfpo
Rl2K9clVMVG7DKcDAhkj4lWsJEaNszvc/3mgGIOhYc4PY1KMN6BVWN+SOtIk2EEW
1YqY8NWi0TxUxJ6J1gZKUJTU8TPO5DZw0nnw0YEckaASur34Jklh+q4tXQzFgX1w
9ZqGNDUa9slCRFy+itfGMtjxQ7dsVntCocw/kEYAlhtfJ7ak5v/IVJFxv5UPHvcB
nevRfUsKzN5Llwh49xVRin8BqtlH6292fcT04DXts4tSdmOxCcVjwt4blcmod03P
LCoYCAh09Y7srm9U0g/I+T391lsRijU+YYGwT9dm9OsUiZ1FgdXpac9Z45UgP9el
SFGMG5hNarFVU4KM/l67e/mT3lFJpOuF6aym7Eh10PYMAiabqGAkETANVtNqd1JY
2UE3CXf8qkFl65XqfLfATchWkF6GcL3KqSenykclS/wMPsEeVvE=
=vjIY
-----END PGP SIGNATURE-----

Show platform data

Tested on Linux-4.19.0-10-amd64-x86_64-with-glibc2.28

Configured with ./configure LDFLAGS=-L/home/james/src/bitcoin/db4/lib/ CPPFLAGS=-I/home/james/src/bitcoin/db4/include/ CXXFLAGS=-fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis --enable-wallet --enable-debug --with-daemon --enable-natpmp-default CXX=/usr/lib/ccache/clang++ CC=/usr/lib/ccache/clang PYTHONPATH= --with-bignum=no --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --disable-shared --with-pic --enable-benchmark=no --enable-module-recovery --enable-module-schnorrsig --enable-experimental --no-create --no-recursion

Compiled with /usr/bin/ccache /usr/lib/ccache/clang++ -std=c++17 -mavx -mavx2 -fPIE -pipe -O2 -g -Wthread-safety-analysis -Wall -Werror=sign-compare -Wsign-compare -Werror=thread-safety-analysis -O0 -g3 -ftrapv -fdebug-prefix-map=$(abs_srcdir)=.  -Wstack-protector -fstack-protector-all -fcf-protection=full -msse4 -msha -msse4.1 -msse4.2  i

Compiler version: clang version 7.0.1-8+deb10u2 (tags/RELEASE_701/final)

@DrahtBot
Copy link
Contributor

DrahtBot commented Jun 17, 2021

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #15936 (Unify bitcoin-qt and bitcoind persistent settings by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

Copy link
Contributor

@ariard ariard left a comment

Choose a reason for hiding this comment

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

IIUC, in case of non-implemented interfaces maker in server-side programs, is this moving failure from server back to client-side ? See my comment.

src/init/bitcoind.cpp Show resolved Hide resolved
Use interfaces::Init::make* methods instead of interfaces::Make*
functions, so interfaces can be constructed differently in different
executables without having to change any code. (So for example
bitcoin-gui can make an interfaces::Node pointer that communicates with
a bitcoin-node subprocess, while bitcoin-qt can make an interfaces::Node
pointer that starts node code in the same process.)
@ryanofsky
Copy link
Contributor Author

Rebased c50facd -> e4709c7 (pr/ipc-make.5 -> pr/ipc-make.6, compare) due to conflict with bitcoin-core/gui#381

@ryanofsky
Copy link
Contributor Author

@jamesob @ariard could reack this since it has been rebased? The only change was to fix a conflict in Qt includes https://github.com/ryanofsky/bitcoin/compare/pr/ipc-make.5-rebase..pr/ipc-make.6

This PR is just replacing interfaces::MakeNode() function calls with interfaces::Init::makeNode() virtual method calls, interfaces::MakeChain() function calls with interfaces::Init::makeChain() virtual method calls, interfaces::MakeWalletClient() function calls with interfaces::Init::makeWalletClient() virtual method calls, interfaces::MakeNode() function calls with interfaces::Init::makeEcho() virtual method calls, so in #10102, the interfaces::Init implementation for the current executable will be able to decide whether to instantiate each interface locally in the current process or remotely over IPC in spawned processes.

Copy link
Contributor

@benthecarman benthecarman left a comment

Choose a reason for hiding this comment

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

utACK e4709c7

@jamesob
Copy link
Member

jamesob commented Sep 8, 2021

reACK e4709c7

Cloned changes and verified interdiff. Only changes are fixing qt imports.

@fanquake fanquake requested a review from ariard September 9, 2021 00:29
@achow101
Copy link
Member

ACK e4709c7

@ryanofsky
Copy link
Contributor Author

Is this ready for merge? Trivial change with multiple acks

@fanquake fanquake merged commit 528e081 into bitcoin:master Sep 16, 2021
@fanquake
Copy link
Member

Looks like this has broken the CI. https://github.com/bitcoin/bitcoin/runs/3616387813:

PASS   : WalletTests::cleanupTestCase()
Totals: 3 passed, 0 failed, 0 skipped, 0 blacklisted, 2431ms
********* Finished testing of WalletTests *********
********* Start testing of AddressBookTests *********
Config: Using QtTest library 5.12.11, Qt 5.12.11 (arm-little_endian-ilp32-eabi-hardfloat static release build; by GCC 8.3.0)
PASS   : AddressBookTests::initTestCase()
test_bitcoin-qt: node/interfaces.cpp:266: node::{anonymous}::NodeImpl::walletClient()::<lambda()>: Assertion `"m_context->wallet_client" && check' failed.
FAIL qt/test/test_bitcoin-qt (exit status: 134)

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 16, 2021
It looks like this should have been caught by CI but perhaps there was a
conflict with a recently merged PR. Failure reported by fanquake <fanquake@gmail.com>
bitcoin#22219 (comment)
@ryanofsky
Copy link
Contributor Author

Looks like this has broken the CI. bitcoin/bitcoin/runs/3616387813:

Sorry about this. I reproduced this locally and posted a fix in #22992. I think there was a silent conflict, maybe with #19101 that prevented this getting caught by CI

ryanofsky added a commit to ryanofsky/bitcoin that referenced this pull request Sep 16, 2021
It looks like this should have been caught by CI but perhaps there was a
conflict with a recently merged PR. Failure reported by fanquake <fanquake@gmail.com>
bitcoin#22219 (comment)
fanquake added a commit to bitcoin-core/gui that referenced this pull request Sep 16, 2021
865ee1a Fix Qt test broken by #22219 (Russell Yanofsky)

Pull request description:

  It looks like this should have been caught by CI but there might have been a conflict with a recently merged PR like #19101. Failure was reported by fanquake bitcoin/bitcoin#22219 (comment)

  Fix avoids null WalletClient pointer dereference in address book test by adding MakeWalletClient call and making address book test initialization more consistent with wallet test initialization:

  https://github.com/bitcoin/bitcoin/blob/865ee1af201238f48671e3b79b8215f896db7a05/src/qt/test/addressbooktests.cpp#L63-L66
  https://github.com/bitcoin/bitcoin/blob/865ee1af201238f48671e3b79b8215f896db7a05/src/qt/test/wallettests.cpp#L141-L144

ACKs for top commit:
  fanquake:
    ACK 865ee1a - I'm merging this now to unbreak the build.

Tree-SHA512: 1f32b7fc79fa79fcf8600d23063896cbc7f8bbcff39d95747ecd546e754581f0f36ece3098ddecded175afccbb3709b4232da39a400dda23b7e550f361b515fb
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 19, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Sep 19, 2021
It looks like this should have been caught by CI but perhaps there was a
conflict with a recently merged PR. Failure reported by fanquake <fanquake@gmail.com>
bitcoin#22219 (comment)
hebasto added a commit to hebasto/gui-qml that referenced this pull request Sep 28, 2021
This change allows merge bitcoin/bitcoin#22219 without conflicts.
hebasto added a commit to hebasto/gui-qml that referenced this pull request Sep 28, 2021
This change allows to merge bitcoin/bitcoin#22219 without conflicts.
hebasto added a commit to bitcoin-core/gui-qml that referenced this pull request Oct 3, 2021
…in/bitcoin#22219

498a318 Revert 8efd330 (Hennadii Stepanov)

Pull request description:

  This PR reverts the _"refactor: Move qwidget and qml common code into the main() function"_ commit (8efd330) from #5, and it allows to sync with the main repo without a merge conflict with bitcoin/bitcoin#22219.

ACKs for top commit:
  promag:
    Tested ACK 498a318, a clean merge is possible after this.

Tree-SHA512: b1d3708a20f36b81225a3c25963dbc32c3a33e54a795184cfea5b4af9c98ecd68cee8d297e94cced3f118aed738bafd4312536d62a445d09626d7fad613a2323
hebasto added a commit to bitcoin-core/gui-qml that referenced this pull request Oct 4, 2021
ef05ff5 qml: Implement #22219 changes in the qml/bitcoin.cpp (Hennadii Stepanov)

Pull request description:

  This PR implements bitcoin/bitcoin#22219 changes in the `qml/bitcoin.cpp`.

ACKs for top commit:
  jarolrod:
    tACK ef05ff5

Tree-SHA512: 571cfb3cac9f01a38f6b22589bb357d23e54b9ebd2bcf0ff4e6a45b89ed72502daab4c3f9549f1c40eec66531f8be57d7cbe827d798bc9007fecb93b5612d3b0
janus pushed a commit to BitgesellOfficial/bitgesell that referenced this pull request Nov 11, 2021
It looks like this should have been caught by CI but perhaps there was a
conflict with a recently merged PR. Failure reported by fanquake <fanquake@gmail.com>
bitcoin/bitcoin#22219 (comment)
@bitcoin bitcoin locked and limited conversation to collaborators Jan 2, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
Development

Successfully merging this pull request may close these issues.

8 participants