Bugfix: Workaround UniValue push_back(bool) limitation with push

luke-jr commented at 6:57 am on July 7, 2021: member

UniValue doesn’t define push_back(bool), so C++ silently converts the bool to int and we end up with a Number of 0/1 instead Explicitly passing a bool UniValue, however, works fine.

Fixes regressions introduced by #20012 and #21302

Bugfix: Workaround UniValue push_back(bool) limitation with push_back(UniValue(bool))

UniValue doesn't define push_back(bool), so C++ silently converts the bool to int and we end up with a Number of 0/1 instead
Explicitly passing a bool UniValue, however, works fine.

5e4b9513be

MarcoFalke commented at 7:06 am on July 7, 2021: member

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.

https://github.com/bitcoin-core/univalue/commit/98fadc090984fa7e070b6c41ccb514f69a371c85

MarcoFalke closed this on Jul 7, 2021

MarcoFalke commented at 8:25 am on July 7, 2021: member

Happy to reopen for more feedback

MarcoFalke reopened this on Jul 7, 2021

hebasto commented at 8:33 am on July 7, 2021: member

Fixes regressions introduced by #20012 and #21302

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

MarcoFalke commented at 8:57 am on July 7, 2021: member

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 https://github.com/bitcoin-core/univalue/commit/98fadc090984fa7e070b6c41ccb514f69a371c85, however it doesn’t seem to address the same issue that was fixed for dictionaries in https://github.com/bitcoin-core/univalue/commit/51d3ab34ba2857f0d03dc07250cb4a2b5e712e67

MarcoFalke commented at 9:15 am on July 7, 2021: member

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.

DrahtBot added the label RPC/REST/ZMQ on Jul 7, 2021

in src/rpc/util.cpp:609 in 5e4b9513be outdated

605@@ -606,8 +606,9 @@ void RPCHelpMan::AppendArgMap(UniValue& arr) const
606             map.push_back(m_name);
607             map.push_back(i);
608             map.push_back(arg_name);
609-            map.push_back(arg.m_type == RPCArg::Type::STR ||
610-                          arg.m_type == RPCArg::Type::STR_HEX);
611+            // NOTE: push_back(bool) converts the bool to Number, so explicitly make a boolean UniValue first

MarcoFalke commented at 11:54 am on July 7, 2021:

This note is only true for broken versions of univalue. push_back in our tree works just fine.

MarcoFalke changes_requested

doc/dependencies: Document UniValue usage 8eba0e1af4

luke-jr commented at 5:22 pm on July 7, 2021: member

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.

DrahtBot commented at 0:52 am on July 8, 2021: 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:

#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.

MarcoFalke commented at 6:11 am on July 8, 2021: member

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.

luke-jr commented at 6:28 am on July 8, 2021: member

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 (https://github.com/jgarzik/univalue/issues/76)

MarcoFalke commented at 6:47 am on July 8, 2021: member

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.

luke-jr commented at 6:59 am on July 8, 2021: member

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.

luke-jr commented at 7:15 am on July 8, 2021: member

To get the ball rolling on eventually not needing this, I opened https://github.com/jgarzik/univalue/pull/83

jnewbery commented at 7:49 pm on July 8, 2021: member

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.

luke-jr commented at 8:05 pm on July 8, 2021: member

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.

MarcoFalke commented at 8:10 pm on July 8, 2021: member

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.

MarcoFalke commented at 8:15 pm on July 8, 2021: member

Ideally, we should just drop the subtree entirely

Related discussion about this happened in at least:

#7349
and #7485

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

luke-jr commented at 8:18 pm on July 8, 2021: member

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.

sipa commented at 8:27 pm on July 8, 2021: member

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.

luke-jr commented at 8:35 pm on July 8, 2021: member

@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.

sipa commented at 8:48 pm on July 8, 2021: member

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. https://github.com/jgarzik/univalue/pull/70 or https://github.com/jgarzik/univalue/pull/79), or that “it’s maintained” - there has been no activity at all.

MarcoFalke commented at 6:52 am on July 9, 2021: member

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”:

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.

Dunlap28 approved

luke-jr commented at 4:09 pm on July 9, 2021: member

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

https://github.com/jgarzik/univalue/pull/58

luke-jr commented at 6:38 pm on July 9, 2021: member

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?

DrahtBot commented at 6:13 pm on July 27, 2021: member

🐙 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”.

DrahtBot added the label Needs rebase on Jul 27, 2021

fanquake commented at 4:00 am on July 30, 2021: member

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.

MarcoFalke commented at 6:40 am on July 30, 2021: member

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

luke-jr commented at 1:12 pm on July 30, 2021: member

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.

fanquake commented at 4:53 am on September 2, 2021: member

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.

fanquake referenced this in commit a7f28af437 on Oct 20, 2021

fanquake commented at 3:06 am on October 20, 2021: member

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.

fanquake closed this on Oct 20, 2021

dekm referenced this in commit c61f8f651d on Oct 27, 2022

DrahtBot locked this on Oct 30, 2022

Bugfix: Workaround UniValue push_back(bool) limitation with push_back(UniValue(bool)) #22412

Conflicts