build: tighter Univalue integration, remove --with-system-univalue

[fanquake]

This PR more tightly integrates building Univalue into our build system. This follows the same approach we use for LevelDB, (Makefile.leveldb.include), and CRC32C (Makefile.crc32c.include), and will be the same approach we use for minisketch; see #23114.

This approach yields a number of benefits, including:

• Faster configuration due to one less subconfigure being run during ./configure i.e 22s with this PR vs 26s
• Faster autoconf i.e 13s with this PR vs 17s
• Improved caching
• No more issues with compiler flags i.e build: compile univalue as c++11 #12467
• More direct control means we can build exactly the objects we want

There might be one argument against making this change, which is that builders should have the option to use "proper shared/system libraries". However, I think that falls down for a few reasons. The first being that we already don't support building with a number of system libraries (secp256k1, leveldb, crc32c); some for good reason. Univalue is really the odd one out at the moment.

Note that the only fork of Core I'm aware of, that actively patches in support for using system libs, also explicitly marks them as "DANGEROUS" and "NOT SUPPORTED". So it would seem they exist more to satisfy a distro requirement, as opposed to something that anyone should, or would actually use in practice.

PRs like #22412 highlight the "issue" with us operating with our own Univalue fork, where we actively fix bugs, and make improvements, when upstream (https://github.com/jgarzik/univalue) may not be taking those improvements, and by all accounts, is not currently actively maintained. Bitcoin Core should not be hamstrung into not being able to fix bugs in a library, and/or have to litter our source with "workarounds", i.e #22412, for bugs we've already fixed, based on the fact that an upstream project is not actively being maintained. Allowing builders to use system libs is really only exacerbating this problem, with little benefit to our project. Bitcoin Core is not quite like your average piece of distro packaged software.

There is the potential for us to give the same treatment to libsecp256k1, however it seems doing that is currently less straightforward.

[DrahtBot]

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #23114 (Add minisketch subtree and integrate into build/test by fanquake)
• #23049 ([WIP] net: implement a StratumV2 Template Provider in core by Fi3)
• #20201 (build: pkg-config related cleanup by hebasto)

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.

[jnewbery]

Strong concept ACK!

As well as the technical benefits listed in the PR description, a strong reason to go in this direction is to lower our maintenance and testing burden. Being able to build with a system univalue library really doesn't provide any benefit to users, but results in issues like #22412 which suck up reviewer and maintainer time. Maintaining support for different versions of univalue also expands the matrix of different build/configuration combinations which we really should (but don't) test if we claim support.

[ryanofsky]

I know that the forking process already began before this PR, and I think that forking in open source can be very good. But I also think that when you fork, it is better practically and ethically to rename the fork. In the case of a library, this doesn't need to involve renaming functions and classes in the library, but the forked library should at least have different library filenames (libunivalue.a) and include filenames (in this case just univalue.h) so it is not a nightmare if downstream projects have code requiring both libraries as dependencies. Also it would be good to rename the source directory src/univalue/ and update the readme documentation src/univalue/README.md to explicity say the library has been forked, the forks are incompatible, problems and discussions about the fork should be directed at the new issue tracker / mailing list / irc channel / whatever, not at the original project.

The idea is that by renaming a directory and two files, you can make things less ambiguous, confusing, and potentially painful for downstream developers, while also being kinder to the author of the original library and letting the name he chose for his project refer unambiguously to his own project instead of a collection of forks.

As for what name to substitute, for an internal fork it could be nonsense like xyzlkq for all it matters, or something like unibtc to be coherent, or something ike jsonvalue to be descriptive, or something like univalue-- to be cheeky, whatever.

Please ignore this suggestion if it's too cumbersome. Just wanted to describe a possible way to make forking less confusing and contentious.

[Rspigler]

Concept ACK. More control over our build is better.

[practicalswift]

Concept ACK for the reasons @jnewbery gave in #22646 (comment)

[laanwj]

Concept ACK.

Please ignore this suggestion if it's too cumbersome. Just wanted to de scribe a possible way to make forking less confusing and contentious.

Not sure about this. As said in the OP, we have done a similar thing for the leveldb and crc32c libraries with regard to maintaining our own fork without renaming them. The subsequent repositories have a mention in their github description that they are not the upstream project, this may be enough. To be clear, it is not the intent that other projects use our fork of the library, it is for our subtree use only. Nor is it intended for them to be packaged (e.g. in Linux distributions). Renaming the project might give a false signal that it's an independent project for general use, somehow.

(Also AFAIK we do still intend to follow upstream changes, and contributing changes upstream, even though they only get merged intermittently if at all, I think it's better regarded as a patch stack on top than a completely separate project)

[maflcko]

Maybe the GitHub projects could be renamed to bitcoin-core/${upstream-name}-subtree to clarify this is a repo only used for bitcoin-core internal subtrees? I agree that renaming the project name itself (in the code) is too much and might even send the wrong impression that this is maintained for someone other than bitcoin-core itself.

[luke-jr]

Concept NACK.

UniValue is not comparable to secp256k1 and LevelDB - the latter are used in consensus-critical code, and so it's important to be careful about which bugs get fixed in them. Using the system UniValue isn't and shouldn't not be dangerous nor unsupported.

There is basically no reason to embed a copy of UniValue at all. If upstream support is lacking, we should just release our fork as a proper fork of the library or not require things upstream has decided not to do. The embedded copy should ideally be removed, and only ever build against the system install.

Other disagree with removing the embedded copy, however. Keeping support for both system and an embedded copy was a reasonable compromise. If we're not going to support both, however, the direction to go is removing the embedded copy, not dropping support for the correct configuration (system library).

[maflcko]

Concept ACK.

Surely, in a world with enough maintenance resources, univalue could be released and integrated as a separate library. As there are evidently neither resources to maintain it as a separate library, nor maintain the integration of an external univalue, this seems like a good move.

[luke-jr]

It's literally less effort to maintain it properly.

All the work to integrate it is already done. The bugs recently introduced, have also been fixed aside from gatekeepers blocking the fixes from being merged.

[laanwj]

Maybe the GitHub projects could be renamed to bitcoin-core/${upstream-name}-subtree to clarify this is a repo only used for bitcoin-core internal subtrees?

Sure, sounds good to me.

Edit: ok, i've renamed:

• bitcoin-core/univalue → bitcoin-core/univalue-subtree
• bitcoin-core/leveldb → bitcoin-core/leveldb-subtree
• bitcoin-core/crc32c → bitcoin-core/crc32c-subtree

The "Subtrees" section in the developer notes likely needs some updates after this.

[jnewbery]

bitcoin-core/univalue-subtree#19 is merged. Does this need rebasing?

[fanquake]

bitcoin-core/univalue-subtree#19 is merged. Does this need rebasing?

I've rebased on master and dropped the cherry-picks in favour of a subtree update. I've also fixed up the integration commit so that make dist is working properly, and made some other minor changes. Also updated the PR description.

[maflcko]

The changes without the last commit will break building with git-bisect? If yes, the changes should probably be squashed into the previous merge commit (git reset --soft HEAD~ && git commit --amend).

[fanquake]

Pushed a fix for the CI failures. The fail*.json sources were missing from the dist tarball, which was causing unitester to fail.

The changes without the last commit will break building with git-bisect?

They shouldn't do. After the subtree pull, univalue is still buildable using autotools. We atomically swap from building univalue using autotools, to integrating univalue into our build in the last commit.

[dongcarl]

Would it be appropriate to split this into other Automake primaries like DATA, HEADERS, etc?

[theuni]

Wouldn't hurt, but it doesn't matter a ton for now since it's all noinst_.

I suspect we will end up needing to make a distinction between headers and source files when we get to c++20 and modules, though.

[fanquake]

I'm going to save this for a followup.

[dongcarl]

ACK 0f95247 less my comment above, always nice to have an include-able sources.mk which makes integration easier.

[DrahtBot]

Guix builds

File	commit 6419bdf (master)	commit adc9284 (master and this pull)
SHA256SUMS.part	57430d4cfea534e7...	90eacc0a4d12e195...
*-aarch64-linux-gnu-debug.tar.gz	9ac5c89fbf2e958a...	0c7a67a6165fd738...
*-aarch64-linux-gnu.tar.gz	fb6ed9d3e3a06da1...	0b7e9c4dc94df909...
*-arm-linux-gnueabihf-debug.tar.gz	bcfe755361d4f4a0...	1d91aea5d456f9bc...
*-arm-linux-gnueabihf.tar.gz	7ae3be5da7e23338...	d0330582b15eb013...
*-osx-unsigned.dmg	e52e0699c7d05349...	e85d267e1ff01bad...
*-osx-unsigned.tar.gz	209f2f2ac192db09...	d130e76ed4560675...
*-osx64.tar.gz	43cc4669c131c052...	3ec532ea9fde0a4b...
*-powerpc64-linux-gnu-debug.tar.gz	31273cba71ae7a63...	b3a0128e22d8bacc...
*-powerpc64-linux-gnu.tar.gz	8459fb85502d8ca1...	bcbdfc167eb9d0e1...
*-powerpc64le-linux-gnu-debug.tar.gz	b15379f1d540d808...	542bee347777c6a6...
*-powerpc64le-linux-gnu.tar.gz	1f8bc012eb1c68ac...	9c375c743824b5e1...
*-riscv64-linux-gnu-debug.tar.gz	13b101bd639bbcb5...	265f85d5ab827cc6...
*-riscv64-linux-gnu.tar.gz	48d281ceade35519...	189f1f62af262a96...
*-win-unsigned.tar.gz	ef39a755917cf4a8...	36e5cd05ece06895...
*-win64-debug.zip	1a513a2f3564f9d7...	0e3d1088b6840df3...
*-win64-setup-unsigned.exe	2da290f0d9af8e78...	e9b1b65c0a143b3f...
*-win64.zip	7405c69d9e088c0a...	c7055eff4e4abd34...
*-x86_64-linux-gnu-debug.tar.gz	67c96b9fe3f5c21e...	4ff19d3d73c6fe7e...
*-x86_64-linux-gnu.tar.gz	bfaa676a253b1279...	91ab2e11b97f90dd...
*.tar.gz	9362f69e52311e85...	aba92a8bd6ece6ca...
guix_build.log	0cd4e97f279ba8fb...	bd39fcce35f4505e...
guix_build.log.diff		e1226a5b02123413...

[theuni]

ACK 0f95247. Thanks fanquake for keeping this going.

There is the potential for us to give the same treatment to libsecp256k1, however it seems doing that is currently less straightforward.

After some discussion in #secp256k1, this probably isn't going to happen. It would require us to essentially copy/paste secp256k1's configure since it deals with c as opposed to c++. I proposed wrapping secp256k1 in a .cpp, but that idea was NACK'd quickly by the upstream maintainers. So I think that subconfigure will probably stay as-is.

[theuni]

Wouldn't hurt, but it doesn't matter a ton for now since it's all noinst_.

I suspect we will end up needing to make a distinction between headers and source files when we get to c++20 and modules, though.

[fanquake]

I'm going to go ahead and merge this. In regards to the comments earlier in this PR, we've done some work to make it more clear that our fork is an internal (for our use only) subtree, by renaming the repository, and I've also opened a PR to adjust the README to mention that our API has diverged from upstream: bitcoin-core/univalue-subtree#30.

After some discussion in #secp256k1, this probably isn't going to happen.

That is slightly disappointing, but at least now we'll be down to only a single extra configure invocation.

Edit: @theuni note that I modified your comment to remove my @ mention so that it wouldn't be included in the merge message.

[jnewbery]

post-merge ACK 0f95247

I've tested the changes and confirmed that the subconfigure isn't run (and that it's slightly faster). I'm not an autotools expert so can only lightly review the diff, but it all looks reasonable to me.

[hebasto]

This PR breaks builds with depends for HOST=x86_64-w64-mingw32 DEBUG=1 due to massive errors like that:

/usr/bin/x86_64-w64-mingw32-ld: ./.libs/libunivalue.a(libunivalue_la-univalue.o):univalue.cpp:(.text+0x890): undefined reference to `__imp_pthread_mutex_lock'
/usr/bin/x86_64-w64-mingw32-ld: ./.libs/libunivalue.a(libunivalue_la-univalue.o):univalue.cpp:(.text+0x8b0): undefined reference to `__imp_pthread_mutex_unlock'

during linking univalue/test/object.exe, univalue/test/unitester.exe and univalue/test/no_nul.exe.

[fanquake]

This PR breaks builds with depends for HOST=x86_64-w64-mingw32 DEBUG=1 due to massive errors like that:

This is the same as #19772.

[hebasto]

This PR breaks builds with depends for HOST=x86_64-w64-mingw32 DEBUG=1 due to massive errors like that:

This is the same as #19772.

The fix has been suggested on #23612.