General discussion on review process, impact on consensus compatible forks, noisy comments

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

There are a few broader issues being bundled into the discussion on the now merged PR #25153 which are leading to some noise and what I consider some inappropriate and uninformed Approach NACKs. I do think these issues warrant further discussion but they are not specific to that particular PR.

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.

I do think Core should factor in the impact of changes to consensus compatible forks such as Knots. It is only one factor of many but I don’t see consensus compatible forks as an irrelevance (especially in the light of motivations of Core subprojects like libbitcoinkernel). Long term I consider consensus compatible forks as an escape latch if a group of maintainers/contributors were ever to go rogue and start merging in things to Core with malicious intentions. To be clear I don’t think this is happening here nor do I think this will happen anytime soon but I would be very uncomfortable with a future where it became increasingly difficult to maintain a consensus compatible fork of Core.

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

There are billions of people who can write “Approach NACK” and then parrot what Luke has said without any understanding of the context of prior Core PRs or the challenges of maintaining a consensus compatible fork of Core. This is noise. Hence for future conversations on broader issues that aren’t specific to a PR I’d ask you to discuss it on IRC, open an issue etc rather than adding noise to the review of a PR with an uninformed Approach NACK. The review process doesn’t work if we start getting billions of parrot NACKs and they somehow have to be factored into the merge decision.

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.

I agree. When there is a technically informed NACK from a maintainer of a consensus compatible fork (and long term contributor to Core) I think at the very least more long term contributors (ideally with contextual understanding of previous PRs) should look at it before it is merged. Ignoring that NACK and merging with a single ACK within 3 days of the PR being opened does not seem like a solid review process to me.

hebasto commented at 2:17 pm on May 20, 2022: member

I do think Core should factor in the impact of changes to consensus compatible forks such as Knots. It is only one factor of many but I don’t see consensus compatible forks as an irrelevance (especially in the light of motivations of Core subprojects like libbitcoinkernel).

Correct, libbitcoinkernel by design assumes to be reused as a library. But Bitcoin Core shouldn’t care about code compatibility with any other bitcoin client. It just creates unneeded engineering burden in this repo.

ghost commented at 2:25 pm on May 20, 2022: none

6gvwih

There are billions of people who can write “Approach NACK” and then parrot what Luke has said without any understanding of the context of prior Core PRs or the challenges of maintaining a consensus compatible fork of Core. This is noise. Hence for future conversations on broader issues that aren’t specific to a PR I’d ask you to discuss it on IRC, open an issue etc rather than adding noise to the review of a PR with an uninformed Approach NACK. The review process doesn’t work if we start getting billions of parrot NACKs and they somehow have to be factored into the merge decision.

I have the understanding of things being discussed in #25153 and do not agree with the way pull request was merged. I do not care if you consider some reviews as noise and it won’t affect my reviews.

michaelfolkson commented at 3:20 pm on May 20, 2022: contributor

But Bitcoin Core shouldn’t care about code compatibility with any other bitcoin client.

This strikes me as opinionated and I’d be interested if other maintainers and long term contributors share this view. You are effectively arguing that there shouldn’t be an escape latch.

e.g. “Long term I consider consensus compatible forks as an escape latch if a group of maintainers/contributors were ever to go rogue and start merging in things to Core with malicious intentions.”

Your viewpoint seems to suggest that if Core was ever corrupted with its current dominance on the network and the perception of it being the reference client then Bitcoin would go down with the Core ship. You wouldn’t care if it was impossible to maintain a fork of Core as that was never a consideration.

hebasto commented at 3:24 pm on May 20, 2022: member

You are effectively arguing that there shouldn’t be an escape latch.

No, I am not.

michaelfolkson commented at 3:27 pm on May 20, 2022: contributor

Ok sorry, I don’t understand your position then. Should Core care whether it is impossible (or extremely difficult) to maintain a consensus compatible fork of Core or should it not? If it shouldn’t care that seems to be arguing that there shouldn’t be a Core escape latch (for now at least). Hopefully libbitcoinkernel will fulfil its aims and make this less of a concern.

luke-jr commented at 6:49 pm on May 20, 2022: member

I would be very uncomfortable with a future where it became increasingly difficult to maintain a consensus compatible fork of Core.

This is already the case in the present. Bitcoin Core’s approaches often almost seem designed to make maintaining derivatives unnecessarily difficult.

But Bitcoin Core shouldn’t care about code compatibility with any other bitcoin client. It just creates unneeded engineering burden in this repo.

Repos don’t do engineering. People do.

It’s one thing to say “I’m not going to go out of the way to avoid breaking X”, it’s another to then go out of the way to break it when there’s no benefit to doing so.

I can continue to provide the minimal patchwork needed to build with standard UniValue before #25153 (which is a best practice, desired and used by people who want Core, not just Knots), but continuing to do so afterward becomes impractical. Since it provides no upsides (unlike #25095 which addresses a real issue), and only breaks compatibility, it should be reverted.

michaelfolkson commented at 7:40 pm on May 20, 2022: contributor

This needs the input of some other long term contributors and/or maintainers then. As far as I can work out 3 maintainers believe Core derivatives/consensus compatible forks of Core should be completely ignored and shouldn’t factor into Core merge decisions to any extent whatsoever. Understandably Luke is unhappy with that as the maintainer of Knots.

achow101 commented at 7:55 pm on May 20, 2022: member

I agree that compatibility with software forks (of any sort) should not be a factor considered when merging things. Hell, we barely consider that for backporting things for our own older releases. In fact, in the past, we have done large breaking refactors specifically right after the branching of a release even though doing so makes it explicitly harder to make backports.

I don’t think it makes a whole lot of sense for us to try to maintain compatibility with upstream UniValue. It doesn’t appear to be maintained at all, and we have had it as a subtree for years already.

kristapsk commented at 9:04 pm on May 20, 2022: contributor

I also think #25153 was merged too fast when there was NACK from @luke-jr with reasoning (does not mean that I’m for or against specific change myself).

MarcoFalke commented at 7:51 am on May 21, 2022: member

I don’t think 25153 should be reverted in any case. If we decide to support forked repos that want to support system univalue it should be done properly with something like #25153 (comment) . If we decide against that, then it shouldn’t be reverted obviously.

Personally I don’t care if 25153 is reverted or not, but I think this whole discussion might be a bit exaggerated. 25153 is a scripted-diff that will run in a few milliseconds. It should take less than 2 minutes for any forked repos to write an inverse scripted-diff which can be executed in the same few milliseconds on any branch they wish to execute it on.

MarcoFalke added the label Brainstorming on May 21, 2022

MarcoFalke commented at 7:54 am on May 21, 2022: member

Closing for now, as this issue is open-ended, but feel free to continue discussion. Also, happy to re-open if someone thinks this is useful.

MarcoFalke closed this on May 21, 2022

luke-jr commented at 8:36 am on May 21, 2022: member

If we decide to support forked repos that want to support system univalue it should be done properly with something like #25153 (comment) . If we decide against that, then it shouldn’t be reverted obviously.

While I obviously prefer fully supporting system univalue, it doesn’t have to be all-or-nothing. Only breaking compatibility as necessary (eg #25095) allows people to patch those few cases. Going out of the way to break it everywhere unnecessarily (#25153) has no upside, and just makes it impractical for anyone to support.

It should take less than 2 minutes for any forked repos to write an inverse scripted-diff which can be executed in the same few milliseconds on any branch they wish to execute it on.

This isn’t sanely distributable or applyable in standard .patch format, and even if one made a giant patch, it would break to combine it with any other modifications.

luke-jr commented at 11:38 am on June 5, 2022: member

Why hasn’t #25153 been reverted yet?

ryanofsky commented at 12:10 pm on June 5, 2022: member

This isn’t sanely distributable or applyable in standard .patch format, and even if one made a giant patch, it would break to combine it with any other modifications.

I still think the original sin is forking the univalue library without renaming it so now there are two libraries with the same name, incompatible, with different maintainers and support channels. IMO the best solution would be to rename libunivalue.a to libbitcoin_json.a, add a comment giving credit and thanks to the original authors, and move on from this.

MarcoFalke commented at 3:12 pm on June 5, 2022: member

Why hasn’t #25153 been reverted yet?

Because no one created a pull request for it yet. And since the pull request doesn’t exists, no one has reviewed it either.

sin is #22646 (comment)

It was renamed to univalue-subtree, see https://github.com/bitcoin-core/univalue-subtree

DrahtBot locked this on Jun 5, 2023

General discussion on review process, impact on consensus compatible forks, noisy comments #25177