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
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | ajtowns, instagibbs |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, 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.
587@@ -588,7 +588,11 @@ void SetupServerArgs(ArgsManager& argsman)
588 argsman.AddArg("-dustrelayfee=<amt>", strprintf("Fee rate (in %s/kvB) used to define dust, the value of an output such that it will cost more than its value in fees at this fee rate to spend it. (default: %s)", CURRENCY_UNIT, FormatMoney(DUST_RELAY_TX_FEE)), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::NODE_RELAY);
589 argsman.AddArg("-bytespersigop", strprintf("Equivalent bytes per sigop in transactions for relay and mining (default: %u)", DEFAULT_BYTES_PER_SIGOP), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
590 argsman.AddArg("-datacarrier", strprintf("Relay and mine data carrier transactions (default: %u)", DEFAULT_ACCEPT_DATACARRIER), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
591- argsman.AddArg("-datacarriersize", strprintf("Maximum size of data in data carrier transactions we relay and mine (default: %u)", MAX_OP_RETURN_RELAY), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
592+ argsman.AddArg("-datacarriersize",
593+ strprintf("Relay and mine transactions whose data-carrying raw scriptPubKey "
594+ "is of this size or less (default: %u)",
595+ MAX_OP_RETURN_RELAY),
596+ ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
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.
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.
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 :)