scripted-diff: Use getInt<T> over get_int/get

MarcoFalke commented at 8:57 am on May 17, 2022: member

Seems better to see the return type directly and be able to modify it easier, as the return type is used for exceptions (in-range checking and parsing feedback).

DrahtBot added the label Refactoring on May 17, 2022

fanquake commented at 3:38 pm on May 17, 2022: member

ACK after rebasing for #23679.

DrahtBot added the label Needs rebase on May 17, 2022

scripted-diff: Use getInt<T> over get_int/get_int64

-BEGIN VERIFY SCRIPT-
 sed -i 's|\<get_int64\>|getInt<int64_t>|g' $(git grep -l get_int ':(exclude)src/univalue')
 sed -i 's|\<get_int\>|getInt<int>|g'       $(git grep -l get_int ':(exclude)src/univalue')
-END VERIFY SCRIPT-

fa9af21878

MarcoFalke force-pushed on May 18, 2022

DrahtBot removed the label Needs rebase on May 18, 2022

luke-jr commented at 6:55 pm on May 18, 2022: member

Concept NACK, this breaks compatibility for no reason and seems to have no upsides. It would appear to serve no purpose other than to try to make more work for others.

MarcoFalke commented at 7:44 pm on May 18, 2022: member

The benefit of the new method is explained in the description. See also the sanitizer fix that fanquake linked to, as well as #25095. Finally, this is a scripted-diff refactor resolving a redirect. Likely the resulting binary doesn’t even change, so I fail to see how this “breaks compatibility”.

fanquake commented at 1:08 pm on May 19, 2022: member

ACK fa9af218780b7960d756db80c57222e5bf2137b1

this breaks compatibility for no reason

Breaks compatibility with what? We have removed support for system univalue, and the API is already marked as broken in our univalue repo:

This is a fork of univalue used by Bitcoin Core. It is not maintained for usage by other projects. Notably, the API is broken in non-backward-compatible ways.

I think we are at the point where we could remove the univalue “subtree” entirely, as having to go through the current cycle of merging changes into our subtree, and then pulling them here, only makes refactoring / cleanups more complicated, and results in us having to carry a stack of additional files we don’t actually need in this repository.

and seems to have no upsides.

There are upsides. They are outlined in the PR description, and two other linked PRs.

It would appear to serve no purpose other than to try to make more work for others.

If by more work, you mean, it might make it more difficult for you to maintain Knots, which maintains build options and code that we have removed, then yes, that’s possible. However Bitcoin Core isn’t making engineering/maintainence decisions based on whether or not they are benficial for forks of this codebase.

DrahtBot commented at 3:31 pm on May 19, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23319 (rpc: Return fee and prevout (utxos) to getrawtransaction by dougEfresh)
#22338 ([Refactor]: Rename Script methods that only work on PreTapScript scripts by sanket1729)
#19391 (RPC/Mining: Clean out pre-Segwit miner compatibility code by luke-jr)
#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.

fanquake merged this on May 19, 2022

fanquake closed this on May 19, 2022

luke-jr commented at 10:17 pm on May 19, 2022: member

@fanquake You are abusing your merge privileges here again. This has 1 ACK (your own), and 1 NACK, and is clearly intended for no purpose other than creating more work for me. Neither the description nor linked PRs give ANY benefits for this PR, which Marco explicitly admits probably doesn’t even change the binary.

ryanofsky commented at 10:56 pm on May 19, 2022: contributor

luke-jr, honestly I tend to have the same opinions as you about these things (how univalue and libraries in general should be treated), but I think direction fanquake and marcofalke want to go, internalizing univalue and targeting it more specifically to our needs, is also reasonable. And the work they are doing is completely invaluable, so I think their opinions should carry a lot of weight in design decisions that affect it, as long as they are reasonable and don’t cause serious problems.

If this change causes more work for you, could something else be done to alleviate it? It might help to know what the specific problem is.

On the code review process and dealing with NACKs, maybe there are some improvements that could be made there, but we should worry about not slowing things down too much and creating more work for maintainers. I imagine reason NACK was not given much weight might have been for reasons described above, or might have just been that it was hard to understand. You didn’t really describe a concrete and easy to understand problem.

luke-jr commented at 11:25 pm on May 19, 2022: member

If this change causes more work for you, could something else be done to alleviate it? It might help to know what the specific problem is.

As mentioned, it breaks compatibility with the official UniValue. If there was a reason or benefit to doing so, I might agree with you that it’s justifiable, but this doesn’t - it’s essentially just a rename that does nothing but break compatibility.

On the code review process and dealing with NACKs, maybe there are some improvements that could be made there, but we should worry about not slowing things down too much and creating more work for maintainers.

Bitcoin Core claims to be a decentralised project, not run on the whims of maintainers. If fanquake wants his own personal derivative where he calls the shots single-handedly, he is welcome to have one.

P.S. Note this isn’t just Knots affected (though the anti-derivative mindset itself is a serious problem); reasonable people packaging Bitcoin Core (not just myself) reasonably wish to use shared libraries.

unknown commented at 11:57 pm on May 19, 2022: none

Post merge NACK

Expecting a better review process followed by bitcoin core repository before merging a PR. Also changes should be reverted if they break compatibility as mentioned by @luke-jr in his review.

MarcoFalke commented at 7:02 am on May 20, 2022: member

I am trying to understand how this pull request breaks compatibility. As I mentioned before it likely has no effect at all on the binary after compilation. So I think you are NACKing the wrong pull request and reverting this commit does not magically reintroduce system library compatibility.

Moreover, I am pretty sure that compatibility was already broken years ago and my attempts to upstream bugfixes to univalue didn’t receive any comment at all in the time. Meaning, the upstream I am aware is unmaintained. So while I agree on the goal of library compatibility, I don’t think it is practically possible in this case.

I am more than happy to revert the changes here if there is an alternative solution to the integer sanitizer crashes we are seeing. My fuzz farm hit this issue #25095 (comment), which is why I created the fix. So any alternative solution (if there is any) should ideally not reintroduce the bug. 25095 has steps to reproduce for both fuzzing and normal RPC usage, but feel free to ping me for testing or review on the alternative solution.

MarcoFalke deleted the branch on May 20, 2022

luke-jr commented at 7:58 am on May 20, 2022: member

I am pretty sure that compatibility was already broken years ago

Prior to this commit, there was a tiny few places Core relied on an API break.

and my attempts to upstream bugfixes to univalue didn’t receive any comment at all in the time.

I’m not aware of any outstanding strict bugfixes, just an undesirable API for pushing booleans.

Meaning, the upstream I am aware is unmaintained.

I agree that appears to be the case. However, its “unmaintained” at this time is simply “stable with no known bugs”.

So while I agree on the goal of library compatibility, I don’t think it is practically possible in this case.

It would in fact be trivial, as I demonstrated in #22412. In fact, it would even be easy to mark push_back(bool) as deprecated in our fork to avoid incompatibilities sneaking back in.

I am more than happy to revert the changes here if there is an alternative solution to the integer sanitizer crashes we are seeing. My fuzz farm hit this issue #25095, which is why I created the fix.

That’s not applicable to this PR at all.

A portable fix would be to use get_int64() and range check it. (Or ToIntegral with getValStr)

hsjoberg commented at 8:07 am on May 20, 2022: contributor

Approach NACK. There’s little reason this has to be rushed into Core, especially after it has received a pretty solid NACK. This “my way or the highway”-attitude reminds me of Gavin Andresen and darker times.

MarcoFalke commented at 8:18 am on May 20, 2022: member

A portable fix would be to use get_int64() and range check it. (Or ToIntegral with getValStr)

I think get_int64() wouldn’t work if you want to get an uint64_t. I guess ToIntegral with getValStr would work better and be cleaner and more consistent. Happy to change to that approach if people think it is worth it. I guess my only worry would be that we’ll start implement/re-implement most of json parsing from univalue outside of the univalue library in our own code. Effectively recreating univalue in our own code over time as more bugs are discovered or more things are reworked. In the future, this may mean we effectively can drop the external dependency at one point completely.

MarcoFalke commented at 8:24 am on May 20, 2022: member

If we go down the route to implement our own int getter to address the incompatibility issue, we could at the same time create forwarding pushers to also address #22412.

fanquake commented at 8:36 am on May 20, 2022: member

My plan is to remove the univalue subtree’ing entirely, dropping all of the code / files we don’t need (most of them), and allowing more reuse / tighter integration with our code. [This is also the approach suggested by @luke-jr in #22646](/bitcoin-bitcoin/22646/#issuecomment-911617873):

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

This would also remove the currently awkward dance of having to send changes “upstream” to our own repository, just to merge, and then pull them down into this repo, and it will allow improvements like https://github.com/bitcoin-core/univalue-subtree/pull/27, to be PR’d directly to this repo, without any deprecation / migration steps (which are only needed due to the existence of the subtree). At this point, the subtree really isn’t serving any purpose, other than given a false sense of us maintaining an (API incompatible) fork of an unmaintained library.

As long as this happens prior to 24.x (I have started work on it already) I think that is ok.

luke-jr commented at 8:39 am on May 20, 2022: member

My plan is to remove the univalue subtree’ing entirely, dropping all of the code / files we don’t need (most of them), and allowing more reuse / tighter integration with our code. This is also the approach suggested by @luke-jr in #22646:

The approach I suggested was to use the shared library exclusively, and to not embed it at all. The complete opposite of the bad idea you are proposing here.

fanquake commented at 8:54 am on May 20, 2022: member

The complete opposite of the bad idea you are proposing here.

What in particular makes it a bad idea, given the current state of our repository? The approach I’ve suggested would remove the need for any shared/external/upstream library entirely, and just integrate the few hundred lines of univalue (JSON parsing) code we actually need, into this repository, while removing all of the cruft from the upstream repo we don’t (build system, docs etc). Then it can be " C++17 modernised", rewritten to improve performance, and take advantage of utility code that we are already maintaining.

laanwj commented at 9:02 am on May 20, 2022: member

Concept ACK. I think it’s overall an improvement to method naming. But I do agree this got merged way too quickly, especially for something that impacts 32 files.

luke-jr commented at 9:07 am on May 20, 2022: member

The approach I’ve suggested would …

JSON parsing/formatting is the job of a JSON library, which should be shared between multiple programs that need JSON capabilities. It isn’t something each JSON-using program should have its own custom code for.

Even within the scope of the project, modularity is good - hence a separate GUI repo, libbitcoinkernel, etc. This approach makes even more sense for logically external libraries.

Ideally, the changes you want should be merged upstream. Given the poor state of upstream, I agree maintaining a fork makes sense - but users should still be able to install the fork (or original) and build Core or other programs using it without compatibility problems.

michaelfolkson commented at 10:27 am on May 20, 2022: contributor

@hsjoberg @1440000bytes: Please open an issue in future to discuss broader issues you may have such as review process concerns on particular PR(s) or whether Core should factor in the impact of changes on consensus compatible forks (e.g. Knots). Personally I share your broader concerns here but they can add noise to the review process on a particular PR. I don’t think either of you (or me for that matter) have anything to add to the technical discussion specific to this PR. We haven’t been following the series of Core PRs leading up to this PR nor are we maintaining a consensus compatible fork of Core and understand the implications of this PR on that task. Hence an Approach NACK from us just adds noise to this PR and is inappropriate.

ghost commented at 12:31 pm on May 20, 2022: none

@michaelfolkson I will comment in the same pull request in future if I have issues with the review of a particular pull request. Your comment adds more noise to the pull request IMO.

We haven’t been following the series of Core PRs leading up to this PR nor are we maintaining a consensus compatible fork of Core and understand the implications of this PR on that task. Hence an Approach NACK from us just adds noise to this PR and is inappropriate.

Its not necessary to follow the history and sometimes common sense is enough to review a pull request. Particularly after reading such tweet from a respected core developer: https://nitter.net/LukeDashjr/status/1527427645025136641

michaelfolkson commented at 1:34 pm on May 20, 2022: contributor

@1440000bytes: As you insisted here you go: #25177

sidhujag referenced this in commit 7e7a02c1ca on May 28, 2022

bitcoin locked this on Jul 22, 2023

scripted-diff: Use getInt over get_int/get_int64 #25153

Conflicts