doc: revert clarify -datacarriersize #29173
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
|
ACK |
|
NACK. Docs should describe what the code does, not what we might like it to do. |
I changed the documentation to match the code in June. Otherwise, there would be a mismatch between the code and the documentation. I am not sure why it would be beneficial to re-introduce the mismatch. I have no objection to changing the code (along with the documentation). However, I think it would be better to change the documentation in the same pull request that also comes with the code changes. A pull request to change the code was opened after the documentation update, so it would make sense for that pull request to also update the documentation. |
|
ACK. The change that is being reverted was a covert attempt at shrugging off the responsibility of addressing CVE-2023-50428 in Bitcoin Core. There is even an open PR to address it, #28408. Exploits must be addressed by changing the C++ code, not the project's documentation. |
This PR was opened and the CVE was filed after the documentation change, so it is not possible that the documentation change was made in response to either of those. In any case, Bitcoin Core is fully open source an no one is holding anyone back from changing the source code, proposing changes, or (importantly) reviewing changes. Generally a PR should be reviewed before merge, also the tests must be passing. No one is holding anyone back from reviewing the pull request. So anyone wanting the PR to progress, should help by spending time on code review. In any case, the CI test failed as soon as the pull request was opened (#28408 (comment)) and as of right now, the "CI failed" label is still on the pull request, so it can not be merged, unless (at a minimum) the tests are passing. |
|
NACK, this re-introduces an inconsistency. |
|
Concept NACK
This is not what it currently does. |
|
@TheCharlatan @brunoerg your concerns are easily addressed by reviewing and merging the PR with the actual bugfix, #28408. |
|
ACK. Good job catching a developer trying to sweep an important issue under the rug. Very concerning tbh. |
|
NACK, the current documentation is more accurate to what the code actually does. If you want to change what the code does, that's already its own PR #28408. |
|
@glozow I see, the status quo only applies when it suits you... It's 50/50 of (N)Ack, it's not what I call an uncontrovesial change... You do well to mention this PR because strangely it was not merged because of this piece of text. |
|
@Retropex She is right, if you want to revert doc, code should also be changed to reflect that. |
|
The purpose of However, a bug to bypass this function was found on December 14, 2022. Literally no developer in the world corrects flaws by changing the documentation. |
|
You never change software requirements to fit the functionality. You either revisit the requirements to "Build the right software" or make the fix to "Build the software right". There was no bug fix but a lazy man's attempt to hide the bug at best. Honestly this person would be reprimanded for such negligence. Put the textual description back to what it was and provide a fix. Ya'll puttin development to shame. |
|
NACK. Rough consensus on behaviour change can be achieved on the other pull, we shouldn't have incorrect documentation while that process plays out. |
The help text and documentation is not generally a statement of requirements, rather it documents what the software does, whether that's optimal/desirable or not. The closest thing we have to requirements docs are the BIPs, however the senior editor there considers node policies like this as being out of scope there as well. |
That is a shame. What the software does and more importantly what the software doesn't do is the very role of software requirements. So first of all get the act together with best software practices. You have a descriptive argument called datacarriersize that doesn't really leave room for debate on what it is supposed to do. It isn't called scriptPubKeySize. However the behavior of this variable was changed without changing the name of the variable. Sneaky, well played. 6 years later Maria updates "docs" by updating the description of the argument. I'm calling out deception and incompetence where it is. Let the facts speak for themselves. |
|
It's a disgrace that core devs with merge rights are behaving as kids and not taking this PR seriously. Please open this issue again and address the concerns with valid arguments rather than acting tyrannical and closing the issue. |
The latest update of the help text of
-datacarriersizeis incorrect.The purpose of this standardization rule is not to target only the data contained in the raw scriptPubKey, but all forms of arbitrary data.
It is incorrect to change the description of this option if an attempt to update it was made without being merged.
Context:
The first inscription appeared December 14, 2022 at the height of block 767430.
@luke-jr released a first quick patch on February 1, 2023
Then a second patch, this time eligible for a merge, was released on September 5, 2023
Finally @maflcko changed the description on June 8, 2023.