doc: revert clarify -datacarriersize

Retropex commented at 8:32 am on January 4, 2024: none

The latest update of the help text of -datacarriersize is 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.

doc: revert clarify -datacarriersize dfd58f5ac1

DrahtBot commented at 8:32 am on January 4, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	ajtowns, TheCharlatan, brunoerg
Concept ACK	ChrisMartl, 1ma, Sun0fABeach

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

DrahtBot added the label Docs on Jan 4, 2024

ChrisMartl commented at 8:38 am on January 4, 2024: none

ACK

ajtowns commented at 8:49 am on January 4, 2024: contributor

NACK. Docs should describe what the code does, not what we might like it to do.

maflcko commented at 8:50 am on January 4, 2024: member

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.

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.

1ma commented at 9:13 am on January 4, 2024: none

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.

maflcko commented at 9:34 am on January 4, 2024: member

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 (https://github.com/bitcoin/bitcoin/pull/28408#event-10278730964) 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.

TheCharlatan commented at 9:51 am on January 4, 2024: contributor

NACK, this re-introduces an inconsistency.

brunoerg commented at 9:59 am on January 4, 2024: contributor

Concept NACK

The purpose of this standardization rule is not to target only the data contained in the raw scriptPubKey, but all forms of arbitrary data.

This is not what it currently does.

sp-marcel-hernandez commented at 10:01 am on January 4, 2024: none

@TheCharlatan @brunoerg your concerns are easily addressed by reviewing and merging the PR with the actual bugfix, #28408.

Sun0fABeach commented at 10:10 am on January 4, 2024: none

ACK. Good job catching a developer trying to sweep an important issue under the rug. Very concerning tbh.

glozow commented at 10:20 am on January 4, 2024: member

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 closed this on Jan 4, 2024

Retropex commented at 10:59 am on January 4, 2024: none

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

kristapsk commented at 11:10 am on January 4, 2024: contributor

@Retropex She is right, if you want to revert doc, code should also be changed to reflect that.

Retropex commented at 11:18 am on January 4, 2024: none

The purpose of -datacarriersize has ALWAYS been, as its name suggests, to enable or deactivate the relay of data-carrying transactions.

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.

crediblebytes commented at 11:34 am on January 4, 2024: none

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.

stickies-v commented at 11:59 am on January 4, 2024: contributor

NACK. Rough consensus on behaviour change can be achieved on the other pull, we shouldn’t have incorrect documentation while that process plays out.

ajtowns commented at 12:54 pm on January 4, 2024: contributor

You never change software requirements to fit the functionality.

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.

crediblebytes commented at 2:29 pm on January 4, 2024: none

The help text and documentation is not generally a statement of requirements

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.

desi-bitcoiner commented at 3:46 pm on January 4, 2024: none

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.

bitcoin locked this on Jan 4, 2024

bitcoin unlocked this on Jan 5, 2024

Retropex deleted the branch on Jan 24, 2024

doc: revert clarify -datacarriersize #29173

Code Coverage

Reviews