Thank you for your contribution. However we do not accept patches that are only meant for downstream projects that are shipping modified versions of Bitcoin Core. This project's goal is to maintain the vanilla version of Bitcoin Core.

bitcoin-core/univalue-subtree@98fadc0

Happy to reopen for more feedback

Fixes regressions introduced by #20012 and #21302

Do we need tests to prevent such regressions in the future?

Agree that this needs tests if it is decided that the workaround should be applied. Also the minimum dependency version should be properly documented in https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md#dependencies to have an objective criteria to decide on what workarounds are acceptable and which ones are not. Currently it is not documented and I (incorrectly?) assumed that only the bundled univalue is supported. The lack of clarification will lead to subjective disagreement about what versions are supported.

Finally, the patch seems incomplete anyway. It is a workaround for the fix for arrays I submitted in bitcoin-core/univalue-subtree@98fadc0, however it doesn't seem to address the same issue that was fixed for dictionaries in bitcoin-core/univalue-subtree@51d3ab3

To reply to my own comment: The minimum required univalue version is hidden in the configure.ac. The last time it was bumped was #12666 to require the bool dictionary fix.

So Concept NACK on this pull. I'd prefer to bump the minimum univalue dependency to require the bool array fix. This is a reliable fix for the bug (unlike this pull request) that also prevents the bug from silently appearing again.

However we do not accept patches that are only meant for downstream projects that are shipping modified versions of Bitcoin Core.

Many changes that benefit other projects (eg, Liquid) have been accepted into Core. This particular fix, however, affects Core itself, without modifications.

Do we need tests to prevent such regressions in the future?

It would be good to get one of the CI tasks using official UniValue, yes. That would have caught this. However, many of our tests don't differentiate between Number and boolean JSON return values, and would have ignored the issue if the bug hadn't been in the test itself.

Personally, I have modified my UniValue to give a deprecation warning when push_back(bool) is used. That could be put in our UniValue fork - or perhaps there's a way to make it stronger (ie, an error)?

Also the minimum dependency version should be properly documented in https://github.com/bitcoin/bitcoin/blob/master/doc/dependencies.md

Done

So Concept NACK on this pull. I'd prefer to bump the minimum univalue dependency to require the bool array fix. This is a reliable fix for the bug (unlike this pull request) that also prevents the bug from silently appearing again.

For that approach, the fix needs to be released upstream first. AFAIK, you have never submitted it upstream (despite me bringing it to your attention previously), so that hasn't even begun...

Until that happens, this fix is needed.

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

• tracing: first tracepoints and documentation on User-Space, Statically Defined Tracing (USDT) #22006 by 0xB10C

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.

AFAIK, you have never submitted it upstream (despite me bringing it to your attention previously), so that hasn't even begun...

Assuming others are obliged to do something is not how open source dev works.

Regardless, I just looked into this and it seems this issue is fixed in the latest release? At least the code where the patch could be applied is removed.

Assuming others are obliged to do something is not how open source dev works.

I didn't assume that. It's simply what would be necessary to avoid this workaround.

Regardless, I just looked into this and it seems this issue is fixed in the latest release? At least the code where the patch could be applied is removed.

It's still an issue in the latest stable 1.0.5, and 1.1.1 (a development release, possibly with a new API) can't be used to build Core at all (jgarzik/univalue#76)

In my view there are two options:

• The univalue upstream that you are using is unmaintained and/or dropped support for Bitcoin Core. In this case we should simply remove --with-system-univalue, because there is no long-term prospect in keeping it.
• It is still maintained. In that case, the bug(s) should be fixed upstream and the minimum version be bumped in our build system to require the bugfix to be included. Reporting the bug upstream to the dependency you are using also seems a more logical first step to me than to craft a workaround.

In that case, the bug(s) should be fixed upstream and the minimum version be bumped in our build system to require the bugfix to be included.

IMO this is the ideal approach to take.

Reporting the bug upstream to the dependency you are using also seems a more logical first step to me than to craft a workaround.

It is unclear if this is going to happen before 22.0.

To get the ball rolling on eventually not needing this, I opened jgarzik/univalue#83

In this case we should simply remove --with-system-univalue, because there is no long-term prospect in keeping it.

Strong concept ACK. The upstream https://github.com/jgarzik/univalue/ project is barely maintained.

In this case we should simply remove --with-system-univalue, because there is no long-term prospect in keeping it.

Strong concept ACK. The upstream https://github.com/jgarzik/univalue/ project is barely maintained.

Strong concept NACK. It appears to be maintained just fine. The only reason this has broken is because @MarcoFalke has gone out of his way to create a divergent fork in the Core subtree for no benefit.

Ideally, we should just drop the subtree entirely, but at the very least, continuing to support building with official UniValue releases is a bare minimum level of competence we should keep.

The only reason this has broken is because @MarcoFalke has gone out of his way to create a divergent fork in the Core subtree for no benefit.

This is false; I did not create the repo that feeds the subtree. Also, it is irrelevant who created the repo, it was created so that bugfixes can be merged faster into the subtree.

In fact, as you can see from all the cross-merge commits, I was putting effort into keeping them in sync.

Ideally, we should just drop the subtree entirely

Related discussion about this happened in at least:

• Build against system UniValue when available #7349
• and Use system univalue by default #7485

Re-hashing the same topic every year isn't going to help this project.

What I see, is that you made an incompatible API change, ignored efforts to reconcile the official UniValue tree, rudely insta-closed this PR to fix compatibility, and when pushed here, you created a PR for it with 15 unrelated commits that you know you would never merge to Core.

Re-hashing the same topic every year isn't going to help this project.

So don't rehash it. We had something that worked. You're the one who apparently wants to change that.

Sorry @luke-jr, but this is ridiculous. Upstream is obviously unmaintained. It seems the maintainer has not had any comments/commits or other activity in over two years, while there have been PRs opened and commented on on a semi-regular basis. Bitcoin Core has had its own fork for a while, because the maintenance situation was sporadic before for time before already, but at this point there is no reason to keep treating that repo as upstream - it's dead. And the fact that it works without maintenance is not a reason why improvements to our fork can't be made. So I think we should simply start treating the https://github.com/bitcoin-core/univalue repository as upstream. You can't go call clear improvements to the codebase "going out of your way to create a divergent fork" when upstream isn't accepting any changes at all.

@sipa There has been nothing to merge during that time. I glanced at the PRs. Some are enhancements tagged for a future release. None are merged in the Core subtree (aside from the ones just opened yesterday), so if upstream is unmaintained, so is Core's fork.

So I think we should simply start treating the https://github.com/bitcoin-core/univalue repository as upstream.

IF upstream is in fact unmaintained, that would seem like a good alternative solution. However, as things stand, that repo breaks compatibility by removing the std::pair stuff, and the README explicitly recommends using upstream instead. To start treating the Core fork as the official UniValue, we should probably add back the std::pair stuff (maybe with an #ifndef so we can avoid using it in Core) and update the README and tag a new version (which configure can then check for).

Edit: We should probably also merge in the bugfix(es) in upstream that Core's fork is still missing... (ironic to complain upstream hasn't acted in 1 day, when we're missing at least one 2+ year old bugfix)

You can't go call clear improvements to the codebase "going out of your way to create a divergent fork" when upstream isn't accepting any changes at all.

That isn't established yet. Upstream hasn't had more than 1 day to triage the push_back change.

You can't go call clear improvements to the codebase "going out of your way to create a divergent fork" when upstream isn't accepting any changes at all.

That isn't established yet. Upstream hasn't had more than 1 day to triage the push_back change.

I assumed you were talking about earlier changes when making that accusation. If that's not the case, I take my words back on that aspect.

I still disagree that there aren't any useful improvements PRed (e.g. jgarzik/univalue#70 or jgarzik/univalue#79), or that "it's maintained" - there has been no activity at all.

you created a PR for it with 15 unrelated commits that you know you would never merge to Core.

This is also false. Both the forks did merge PRs with "unrelated commits":

• Fix BOOST_CHECK_THROW jgarzik/univalue#57
• Merge changes from jgarzik/univalue@1ae6a23 bitcoin-core/univalue-subtree#23

Merging one branch into another (with all commits) is one way to keep them in sync.

Edit: We should probably also merge in the bugfix(es) in upstream that Core's fork is still missing... (ironic to complain upstream hasn't acted in 1 day, when we're missing at least one 2+ year old bugfix)

It might be good to explain which fixes you are referring to.

IF upstream is in fact unmaintained, that would seem like a good alternative solution. However, as things stand, that repo breaks compatibility by removing the std::pair stuff, and the README explicitly recommends using upstream instead. To start treating the Core fork as the official UniValue, we should probably add back the std::pair stuff (maybe with an #ifndef so we can avoid using it in Core) and update the README and tag a new version (which configure can then check for).

There is no maintained version of Bitcoin Core that uses std::pair wrapper. Simply bumping the version to indicate a breaking change would be easier than adding it back.

It might be good to explain which fixes you are referring to.

jgarzik/univalue#58

Mr. Garzik advises that he still maintains UniValue and will make a new stable release (presumably with the fix) this month.

Perhaps the best approach at this time, is to simply bump the minimum version required to 1.0.6 assuming it will be available by the time 22.0 is final?

🐙 This pull request conflicts with the target branch and needs rebase.

_{Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".}

Mr. Garzik advises that he still maintains UniValue and will make a new stable release (presumably with the fix) this month.

It's now essentially the end of the month, and I can't yet see any new activity in https://github.com/jgarzik/univalue.

In this case we should remove --with-system-univalue. See #22412 (comment)

Or just fix the bug in Core. Upstream not agreeing to your API change is not an excuse to break compatibility.

If you want to become upstream, fine, but don't be a jerk to everyone who wishes to use proper shared libraries. NACK to removing UniValue support and creating divergent vendor-only forks for no reason.

Concept NACK. I agree with the discussion here, in regards to why this is not something we want to, or should have to do. I've summarized my thoughts in #22646, which removes support for system univalue.

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.

Also

It's now essentially the end of the month, and I can't yet see any new activity in https://github.com/jgarzik/univalue.

Another month has passed and there's neither release activity, not commentary on any of the new PRs by Bitcoin Core contributors in the https://github.com/jgarzik/univalue/ repo.

I'm closing this now that #22646 has been merged. For the record, there still doesn't seem to be any new activity upstream, and this is now nearly 3 months after upstream advised they were going to make a new release.