Revert "Merge #16367: Multiprocess build support"

MarcoFalke commented at 11:44 pm on April 10, 2020: member

Reverting the changes temporarily is going to help with the following:

Discussion about the next steps for the multiprocess concept and the experimental libmultiprocess library without having code already commited in the master branch, potentially influencing the discussion
Allowing for more conceptual as well as code review ACKs to accumulate, since the pull only had one ACK (two if I count mine, which didn’t make it to GitHub)

Can be reviewed with git diff HEAD HEAD~2 | wc or git diff 1b307613604883daea4913a65da30ae073c9dc4d~ | wc (should be all zeros)

Context here: #16367 (comment)

Revert "Merge #16367: Multiprocess build support"

This reverts the changes made in merge commit
1b307613604883daea4913a65da30ae073c9dc4d:

This reverts commit b919efadff3d0393f4a8c3c1dc735f7ac5c665bb.
This reverts commit d54f64c6c700be0604190f52c84fc5f1cdd9f02f.
This reverts commit 787f40668dc15980c3d6de028d7950c08175d84a.
This reverts commit d6306466626635e6fee44385e6a688c8dc118eb5.
This reverts commit e6e44eedd56ecaf59f3fabf8e07ab7dee0ddb1b6.

f29bd546ec

MarcoFalke added the label Build system on Apr 10, 2020

MarcoFalke assigned fanquake on Apr 10, 2020

DrahtBot commented at 4:04 am on April 11, 2020: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#10102 ([experimental] Multiprocess bitcoin 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.

ryanofsky approved

ryanofsky commented at 11:55 am on April 11, 2020: member

Code review ACK f29bd546ec169dd9af2ca6265c76353e68db92be. Confirmed revert with

0git checkout $(git merge-base origin/master origin/pull/18588/head)
1git revert --no-edit $(git rev-list --min-parents=2 --max-count=1 origin/pull/16367/head)..origin/pull/16367/head
2git diff origin/pull/18588/head

fanquake approved

fanquake commented at 1:19 pm on April 11, 2020: member

ACK f29bd546ec169dd9af2ca6265c76353e68db92be

I’ve been thinking more about this a bit more, and I think reversion is the correct decision. There seem to be (at least for me) some unanswered questions, and no clear path for how this integration is going to work, so I think merging was premature.

I might have missed these discussions, but some of the things I’d like to know / discuss include:

What’s the expectation for when this should/would be turned on by default? If this remained merged now, would it be on by 0.22? Or is the thinking an even later release?
How will Cores build/compatibility requirements change when we adopt this?

libmultiprocess currently requires C++17 (although Russ has mentioned that it’d be possible to drop back to a C++11 if necessary).
If the install method is clone and build, users would need autotools and cmake to build Core.
I assume capnproto can be installed and “just works” for our purposes everywhere we’d expect anyone to be building Core?
It was pointed out that building capnproto from source currently requires a newer compiler than what we do for building Core.
It looks like there are already some back-compat patches being applied for the version of capnproto that ships with Ubuntu Bionic (18.04). Does libmultiprocess work with the older version of capnproto that ships with Ubuntu Xenial? Is the expectation that some/all users might have to build capnproto from source?

What does integration actually look like:

Will libmultiprocess end up being a submodule?
How will it be versioned / tied to releases of Core?

Alternate approaches?

Apart from that, I might be incorrect here, but it seems we’ve gotten some changes, such as adding a native_boost package, that I’d assume are now unnecessary given that libmultiprocess has dropped it’s Boost usage? That was at least mentioned in 16367, although looking at later comments this is unclear, and it seems the Boost dependency might be coming back anyways?

Misc:

Does the capnproto release schedule/upstream maintainence matter to us? Looking at the repo, it seems that there are at least a couple people calling for a new release (1, 2), and the current maintainer has mentioned that’s he basically too busy at the moment. This might be irrelevant (and it seems that a release prep PR has been merged), but worth noting if this project is going to take on (by extension) a new dependency.

MarcoFalke commented at 1:28 pm on April 11, 2020: member

@fanquake Thanks for the writeup and questions. I think we should go ahead and merge this and then reopen #16367, so that the questions can be discussed in the pull request that introduces the code changes, not the one that reverts them.

MarcoFalke merged this on Apr 11, 2020

MarcoFalke closed this on Apr 11, 2020

theuni commented at 2:08 pm on April 11, 2020: member

Thanks for writing that out @fanquake.

ACK on the revert for now with the understanding that this was simply merged too early, and with a few too many unanswered questions.

MarcoFalke deleted the branch on Apr 11, 2020

ryanofsky commented at 6:46 pm on April 11, 2020: member

Thanks for writing up these questions.

What’s the expectation for when this should/would be turned on by default? If this remained merged now, would it be on by 0.22? Or is the thinking an even later release?

I’m not going to push for this to be on by default. I want this to be an optional feature you can ignore if you’re happy with current bitcoin usage, or use if you want to try new things like starting and stopping gui/node/wallet processes separately or running from different containers or devices.

How will Cores build/compatibility requirements change when we adopt this?

Requirements should not change because this is an optional feature.

If you download a source distribution and want to build ii with --enable-multiprocess, you will currently need a compiler that supports c++17. But this is a recent change that I thought was harmless and could be reversed if it’s a problem. #16367 added a travis variant to make sure building in this configuration works and functional tests pass using the multiprocess binaries.

If we add decide to add new multiprocess binaries bitcoin-gui bitcoin-node and bitcoin-wallet to our binary distributions after #10102, these will work well on mac and linux, but I will need to replace fork/exec/pipe usages in libmultiprocess to make them useful or interesting to run on windows.

libmultiprocess currently requires C++17 (although Russ has mentioned that it’d be possible to drop back to a C++11 if necessary).

Yes, that’s all correct

If the install method is clone and build, users would need autotools and cmake to build Core.

“users would need autotools and cmake to build Core” is only true if you are talking about building the new, optional, disabled-by-default multiprocess bitcoin-gui bitcoin-node and bitcoin-wallet binaries.

I assume capnproto can be installed and “just works” for our purposes everywhere we’d expect anyone to be building Core?

I’m not assuming this. I’m assuming linux/mac/windows support is good but would not be surprised by build or other issues on Android, CloudABI, other less common IDEs or platforms people build bitcoin for. That’s why capnp isn’t a dependency unless you enable an optional feature.

It was pointed out that building capnproto from source currently requires a newer compiler than what we do for building Core.

Only the latest capnp releases require c++14. The previous releases which I haven’t used recently but should still work require c++11.

It looks like there are already some back-compat patches being applied for the version of capnproto that ships with Ubuntu Bionic (18.04). Does libmultiprocess work with the older version of capnproto that ships with Ubuntu Xenial? Is the expectation that some/all users might have to build capnproto from source?

I don’t know if it currently works with the xenial capnp package, but I started with 0.5.3 originally, and if it doesn’t still work, any bugs should be fixable

What does integration actually look like:

Will libmultiprocess end up being a submodule?

How will it be versioned / tied to releases of Core?

Weak preference for not doing whatever we are doing with univalue and leveldb, because I get confused by the 30 different places you can submit univalue pull requests, and because I want to reduce barriers preventing good review. The context hash pinning in #16367 is nice because it’s more straightforward to review a diff of two content hashes than it is to review diffs of diffs like seems to be required to review other dependency update PRs.

I would like to move the libmultiprocess repo from the chaincode org to bitcoin-core and to start signing tags for versioning.

Apart from that, I might be incorrect here, but it seems we’ve gotten some changes, such as adding a native_boost package, that I’d assume are now unnecessary given that libmultiprocess has dropped it’s Boost usage? That was at least mentioned in 16367, although looking at later comments this is unclear, and it seems the Boost dependency might be coming back anyways?

I thought this was covered during #16367 review discussion, and I was thinking native_boost could be useful again, but it’s basically just a handful of variable declarations. Will drop if / when I resubmit #16367

Does the capnproto release schedule/upstream maintainence matter to us? Looking at the repo, it seems that there are at least a couple people calling for a new release (1, 2), and the current maintainer has mentioned that’s he basically too busy at the moment. This might be irrelevant (and it seems that a release prep PR has been merged), but worth noting if this project is going to take on (by extension) a new dependency.

I’m only using basic features, and I believe the release from 5 years ago should still work fine. Also, CapnProto itself shouldn’t be a hangup. I like it very much at this point, but if somebody doesn’t like it, they should be able to ignore it and not use it. If someone wants to use a different protocol as an alternative or replacement, much of the work I’ve done here (interface declarations, process spawning code, build support) should be compatible or reusable for that.

ACK on the revert for now with the understanding that this was simply merged too early, and with a few too many unanswered questions.

Thanks for the questions. I’m sure there will be more asked, but I’ll try to keep the number of unanswered ones close to zero

ryanofsky added this to the "In progress" column in a project

fanquake referenced this in commit 97b21b302a on May 21, 2020

sidhujag referenced this in commit 6a4320e7a9 on May 21, 2020

ryanofsky moved this from the "In progress" to the "Done" column in a project

DrahtBot locked this on Feb 15, 2022

Revert “Merge #16367: Multiprocess build support” #18588

Conflicts