doc: Clarify -datacarriersize, add -datacarriersize=2 tests #27832
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
ACK fa4f476 |
|
Concept ACK |
| strprintf("Relay and mine transactions whose data-carrying raw scriptPubKey " | ||
| "is of this size or less (default: %u)", | ||
| MAX_OP_RETURN_RELAY), | ||
| ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY); |
There was a problem hiding this comment.
NACK This wording is misleading. OP_Return outputs only contain data if they contain something in addition to the OP_Return opcode.
A better approach here if we are touching this opcode could be to redefine -datacarriersize to be size in-addition-too the OP_Return opcode that is allowed. Thus -datacarriersize=0 would allow 0 bytes in addition to the 1 byte used by the OP_Return opcode.
There was a problem hiding this comment.
My goal is to document the current behavior and then behavior changes can be done in a follow-up change. Happy to drop this commit or change to a better wording to describe the current behavior, if you have any suggestions.
There was a problem hiding this comment.
Since you are updating this, you should change the doc text to make clear that the current behavior is a mistake that needs fixing rathre than continue to incorrectly call them data carrier scriptPubKey's.
But it'd be less code churn to just fix the behavior in one shot. We have an ACK on #27261, so there is an argument to fix the bare OP_Return case. Rather than merging #27261 as-is I could make a pull-req that simply fixes the -datacarriersize off-by-one error. Fixing that error would also allow -datacarrier to be dropped entirely, as it is redundant: -datacarriersize=0 prohibits data carrying outputs just fine.
There was a problem hiding this comment.
Fixing that error would also allow -datacarrier to be dropped entirely, as it is redundant: -datacarriersize=0 prohibits data carrying outputs just fine.
I think -datacarrier can be dropped regardless of whether the behavior is changed. And this would have been my review comment in #27261 :)
|
Rebased for green CI |
|
ACK faafc35 |
|
ACK faafc35 |
…ize=2 tests faafc35 doc: Clarify that -datacarriersize applies to the full raw scriptPubKey, not the data push (MarcoFalke) 55550e7 test: Add -datacarriersize=2 tests (MarcoFalke) Pull request description: Clarify with a test that `-datacarriersize` applies to the serialized size of the scriptPubKey, not the size of the pushed data. So for example, * `-datacarriersize=2` will reject a `raw(6a01aa)`, even though only one byte is pushed * `-datacarriersize=0` (or `-datacarrier=0`) will reject a `raw(6a)`, even though no byte is pushed * `-datacarriersize=0` (or `-datacarrier=0`) will reject a `raw(6a00)`, even though zero bytes are pushed ACKs for top commit: ajtowns: ACK faafc35 instagibbs: ACK bitcoin@faafc35 Tree-SHA512: f01ace02798f596ac2a02461e9f2a6ef91b3b37c976ea0b3bc860e2d3efb0ace0fd8b779dd18249cee7f84ebbe5fd21d8506afd3a15edadc00b843ff3b4aacc7
Clarify with a test that
-datacarriersizeapplies to the serialized size of the scriptPubKey, not the size of the pushed data. So for example,-datacarriersize=2will reject araw(6a01aa), even though only one byte is pushed-datacarriersize=0(or-datacarrier=0) will reject araw(6a), even though no byte is pushed-datacarriersize=0(or-datacarrier=0) will reject araw(6a00), even though zero bytes are pushed