Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Remove arbitrary restrictions on OP_RETURN by default #28130

Closed
wants to merge 2 commits into from
Closed

Remove arbitrary restrictions on OP_RETURN by default #28130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7dec186
Fix off-by-one error in data carrier size limit
petertodd Jul 22, 2023
50fc9f6
Remove arbitrary restrictions on OP_RETURN by default
petertodd Jul 23, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions src/init.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,8 +589,7 @@ void SetupServerArgs(ArgsManager& argsman)
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);
argsman.AddArg("-acceptstalefeeestimates", strprintf("Read fee estimates even if they are stale (%sdefault: %u) fee estimates are considered stale if they are %s hours old", "regtest only; ", DEFAULT_ACCEPT_STALE_FEE_ESTIMATES, Ticks<std::chrono::hours>(MAX_FILE_AGE)), ArgsManager::ALLOW_ANY | ArgsManager::DEBUG_ONLY, OptionsCategory::DEBUG_TEST);
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);
argsman.AddArg("-datacarrier", strprintf("Relay and mine data carrier transactions (default: %u)", DEFAULT_ACCEPT_DATACARRIER), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason to remove this option? Seems fine to keep it as an alias to completely disable any OP_RETURN. I know it may be redundant with the other option, but for some reason most other devs and users like the UX of it, similar to -regtest and -chain=regtest, or other options.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I don't think the redundancy is warranted going forwards.
  2. Re: backwards compatibility, I expect that only a very small number of people have set -datacarrier=0, so I'm fine with those few people having to set -datacarriersize=0.

Though if there is strong demand, I could make -datacarrier=0 act as an override on -datacarriersize, forcing the latter to zero.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you also changed -datacarriersize to always be +1, so setting it to zero has a different meaning.

The cost seems low to keep the meaning and features the same, so personally I think it is fine to keep them the same.

In any case, release notes are needed

  • for all removed features,
  • for all changed meanings,
  • for all lifted restrictions, and
  • for all default value changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you also changed -datacarriersize to always be +1, so setting it to zero has a different meaning.

The cost seems low to keep the meaning and features the same, so personally I think it is fine to keep them the same.

The difference in meaning here is what I would consider to be a bug fix. Previously these options also applied to non-data-carrying OP_Return outputs, which doesn't make any sense. Anyone actually using -datacarriersize would just increment the amount accepted by 1 byte, which is insignificant.

In any case, release notes are needed

* for all removed features,

* for all changed meanings,

* for all lifted restrictions, and

* for all default value changes.

Sure, I can write those. I take it the mechanism used by this pull-req is correct? https://github.com/bitcoin/bitcoin/pull/27757/files

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);
argsman.AddArg("-datacarriersize", "Limit the size of data in data carrier transactions we relay and mine", ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
argsman.AddArg("-mempoolfullrbf", strprintf("Accept transaction replace-by-fee without requiring replaceability signaling (default: %u)", DEFAULT_MEMPOOL_FULL_RBF), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
argsman.AddArg("-permitbaremultisig", strprintf("Relay non-P2SH multisig (default: %u)", DEFAULT_PERMIT_BAREMULTISIG), ArgsManager::ALLOW_ANY,
OptionsCategory::NODE_RELAY);
Expand Down
4 changes: 2 additions & 2 deletions src/kernel/mempool_options.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,9 +49,9 @@ struct MemPoolOptions {
* type is designated as TxoutType::NULL_DATA.
*
* Maximum size of TxoutType::NULL_DATA scripts that this node considers standard.
* If nullopt, any size is nonstandard.
* If nullopt, there is no limit on the size or number of outputs.
*/
std::optional<unsigned> max_datacarrier_bytes{DEFAULT_ACCEPT_DATACARRIER ? std::optional{MAX_OP_RETURN_RELAY} : std::nullopt};
std::optional<unsigned> max_datacarrier_bytes{std::nullopt};
bool permit_bare_multisig{DEFAULT_PERMIT_BAREMULTISIG};
bool require_standard{true};
bool full_rbf{DEFAULT_MEMPOOL_FULL_RBF};
Expand Down
6 changes: 1 addition & 5 deletions src/node/mempool_args.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,11 +81,7 @@ util::Result<void> ApplyArgsManOptions(const ArgsManager& argsman, const CChainP

mempool_opts.permit_bare_multisig = argsman.GetBoolArg("-permitbaremultisig", DEFAULT_PERMIT_BAREMULTISIG);

if (argsman.GetBoolArg("-datacarrier", DEFAULT_ACCEPT_DATACARRIER)) {
mempool_opts.max_datacarrier_bytes = argsman.GetIntArg("-datacarriersize", MAX_OP_RETURN_RELAY);
} else {
mempool_opts.max_datacarrier_bytes = std::nullopt;
}
mempool_opts.max_datacarrier_bytes = argsman.GetIntArg("-datacarriersize");

mempool_opts.require_standard = !argsman.GetBoolArg("-acceptnonstdtxn", !chainparams.RequireStandard());
if (!chainparams.IsTestChain() && !mempool_opts.require_standard) {
Expand Down
10 changes: 6 additions & 4 deletions src/policy/policy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,7 @@ bool IsStandard(const CScript& scriptPubKey, const std::optional<unsigned>& max_
if (m < 1 || m > n)
return false;
} else if (whichType == TxoutType::NULL_DATA) {
if (!max_datacarrier_bytes || scriptPubKey.size() > *max_datacarrier_bytes) {
if (max_datacarrier_bytes && scriptPubKey.size() > *max_datacarrier_bytes + 1) {
return false;
}
}
Expand Down Expand Up @@ -136,7 +136,8 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_dat
return false;
}

if (whichType == TxoutType::NULL_DATA)
// data carrying outputs only count if they actually carry data
if (whichType == TxoutType::NULL_DATA && txout.scriptPubKey.size() > 1)
nDataOut++;
else if ((whichType == TxoutType::MULTISIG) && (!permit_bare_multisig)) {
reason = "bare-multisig";
Expand All @@ -147,8 +148,9 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_dat
}
}

// only one OP_RETURN txout is permitted
if (nDataOut > 1) {
// only one data carrying OP_RETURN txout is permitted if datacarrier is
// limited
if (max_datacarrier_bytes && nDataOut > 1) {
reason = "multi-op-return";
return false;
}
Expand Down
6 changes: 0 additions & 6 deletions src/script/standard.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,6 @@ class CScriptID : public BaseHash<uint160>
explicit CScriptID(const ScriptHash& in);
};

/**
* Default setting for -datacarriersize. 80 bytes of data, +1 for OP_RETURN,
* +2 for the pushdata opcodes.
*/
static const unsigned int MAX_OP_RETURN_RELAY = 83;
petertodd marked this conversation as resolved.
Show resolved Hide resolved

/**
* Mandatory script verification flags that all new blocks must comply with for
* them to be valid. (but old blocks may not comply with) Currently just P2SH,
Expand Down
25 changes: 16 additions & 9 deletions src/test/transaction_tests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -764,14 +764,18 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
key.MakeNewKey(true);
t.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key.GetPubKey()));

// 82 bytes of script data after the OP_Return is the old null-data limit.
//
// We use that limit here to allow the data size limit code to be tested
// without redoing all the test cases.
constexpr auto CheckIsStandard = [](const auto& t) {
std::string reason;
BOOST_CHECK(IsStandardTx(CTransaction{t}, MAX_OP_RETURN_RELAY, g_bare_multi, g_dust, reason));
BOOST_CHECK(IsStandardTx(CTransaction{t}, 82, g_bare_multi, g_dust, reason));
BOOST_CHECK(reason.empty());
};
constexpr auto CheckIsNotStandard = [](const auto& t, const std::string& reason_in) {
std::string reason;
BOOST_CHECK(!IsStandardTx(CTransaction{t}, MAX_OP_RETURN_RELAY, g_bare_multi, g_dust, reason));
BOOST_CHECK(!IsStandardTx(CTransaction{t}, 82, g_bare_multi, g_dust, reason));
BOOST_CHECK_EQUAL(reason_in, reason);
};

Expand Down Expand Up @@ -818,14 +822,16 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
t.vout[0].scriptPubKey = CScript() << OP_1;
CheckIsNotStandard(t, "scriptpubkey");

// MAX_OP_RETURN_RELAY-byte TxoutType::NULL_DATA (standard)
// TxoutType::NULL_DATA, with null-data size limited
//
// 82 bytes + 1 byte for the OP_Return is the old standardness limit
t.vout[0].scriptPubKey = CScript() << OP_RETURN << ParseHex("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3804678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38");
BOOST_CHECK_EQUAL(MAX_OP_RETURN_RELAY, t.vout[0].scriptPubKey.size());
BOOST_CHECK_EQUAL(82 + 1, t.vout[0].scriptPubKey.size());
CheckIsStandard(t);

// MAX_OP_RETURN_RELAY+1-byte TxoutType::NULL_DATA (non-standard)
// Same as above, but 1 byte over the limit
t.vout[0].scriptPubKey = CScript() << OP_RETURN << ParseHex("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3804678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3800");
BOOST_CHECK_EQUAL(MAX_OP_RETURN_RELAY + 1, t.vout[0].scriptPubKey.size());
BOOST_CHECK_EQUAL(82 + 1 + 1, t.vout[0].scriptPubKey.size());
CheckIsNotStandard(t, "scriptpubkey");

// Data payload can be encoded in any way...
Expand All @@ -848,21 +854,22 @@ BOOST_AUTO_TEST_CASE(test_IsStandard)
t.vout[0].scriptPubKey = CScript() << OP_RETURN;
CheckIsStandard(t);

// Only one TxoutType::NULL_DATA permitted in all cases
// Only one data-containing TxoutType::NULL_DATA is permitted with the limit enabled
t.vout.resize(2);
t.vout[0].scriptPubKey = CScript() << OP_RETURN << ParseHex("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38");
t.vout[0].nValue = 0;
t.vout[1].scriptPubKey = CScript() << OP_RETURN << ParseHex("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38");
t.vout[1].nValue = 0;
CheckIsNotStandard(t, "multi-op-return");

// However, multiple non-data-containing TxoutType::NULL_DATA's are permitted
t.vout[0].scriptPubKey = CScript() << OP_RETURN << ParseHex("04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38");
t.vout[1].scriptPubKey = CScript() << OP_RETURN;
CheckIsNotStandard(t, "multi-op-return");
CheckIsStandard(t);

t.vout[0].scriptPubKey = CScript() << OP_RETURN;
t.vout[1].scriptPubKey = CScript() << OP_RETURN;
CheckIsNotStandard(t, "multi-op-return");
CheckIsStandard(t);

// Check large scriptSig (non-standard if size is >1650 bytes)
t.vout.resize(1);
Expand Down
1 change: 1 addition & 0 deletions test/functional/mempool_accept.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ def set_test_params(self):
self.num_nodes = 1
self.extra_args = [[
'-txindex','-permitbaremultisig=0',
'-datacarriersize=83',
]] * self.num_nodes
self.supports_cli = False

Expand Down
59 changes: 42 additions & 17 deletions test/functional/mempool_datacarrier.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
"""Test datacarrier functionality"""
from test_framework.messages import (
CTxOut,
MAX_OP_RETURN_RELAY,
)
from test_framework.script import (
CScript,
Expand All @@ -19,19 +18,38 @@
)
from test_framework.wallet import MiniWallet

from typing import List

class DataCarrierTest(BitcoinTestFramework):
def set_test_params(self):
self.num_nodes = 3
self.extra_args = [
[],
["-datacarrier=0"],
["-datacarrier=1", f"-datacarriersize={MAX_OP_RETURN_RELAY - 1}"]
["-datacarriersize=0"],
["-datacarriersize=100"]
]

def test_null_data_transaction(self, node: TestNode, data: bytes, success: bool) -> None:
def test_null_data_transaction(self, node: TestNode, datas: List[bytes], success: bool) -> None:
tx = self.wallet.create_self_transfer(fee_rate=0)["tx"]
tx.vout.append(CTxOut(nValue=0, scriptPubKey=CScript([OP_RETURN, data])))

for data in datas:
tx.vout.append(CTxOut(nValue=0, scriptPubKey=CScript([OP_RETURN, data])))

tx.vout[0].nValue -= tx.get_vsize() # simply pay 1sat/vbyte fee

tx_hex = tx.serialize().hex()

if success:
self.wallet.sendrawtransaction(from_node=node, tx_hex=tx_hex)
assert tx.rehash() in node.getrawmempool(True), f'{tx_hex} not in mempool'
else:
assert_raises_rpc_error(-26, "scriptpubkey", self.wallet.sendrawtransaction, from_node=node, tx_hex=tx_hex)

def test_bare_op_return(self, node: TestNode, n: int, success: bool) -> None:
tx = self.wallet.create_self_transfer(fee_rate=0)["tx"]

for i in range(0, n):
tx.vout.append(CTxOut(nValue=0, scriptPubKey=CScript([OP_RETURN])))
tx.vout[0].nValue -= tx.get_vsize() # simply pay 1sat/vbyte fee

tx_hex = tx.serialize().hex()
Expand All @@ -45,26 +63,33 @@ def test_null_data_transaction(self, node: TestNode, data: bytes, success: bool)
def run_test(self):
self.wallet = MiniWallet(self.nodes[0])

# By default, only 80 bytes are used for data (+1 for OP_RETURN, +2 for the pushdata opcodes).
default_size_data = random_bytes(MAX_OP_RETURN_RELAY - 3)
too_long_data = random_bytes(MAX_OP_RETURN_RELAY - 2)
small_data = random_bytes(MAX_OP_RETURN_RELAY - 4)
# Pushdata opcode takes up 2 bytes
exact_size_data = random_bytes(100 - 2)
too_large_data = random_bytes(100 - 1)
small_data = random_bytes(100 - 3)

self.log.info("Testing null data transaction with default -datacarrier and -datacarriersize values.")
self.test_null_data_transaction(node=self.nodes[0], data=default_size_data, success=True)
self.log.info("Testing null data transaction without -datacarriersize limit.")
self.test_null_data_transaction(node=self.nodes[0], datas=[random_bytes(99800)], success=True)

self.log.info("Testing a null data transaction larger than allowed by the default -datacarriersize value.")
self.test_null_data_transaction(node=self.nodes[0], data=too_long_data, success=False)
self.log.info("Testing multiple null data outputs without -datacarriersize limit.")
self.test_null_data_transaction(node=self.nodes[0], datas=[random_bytes(100), random_bytes(100)], success=True)

self.log.info("Testing a null data transaction with -datacarrier=false.")
self.test_null_data_transaction(node=self.nodes[1], data=default_size_data, success=False)
self.log.info("Testing a null data transaction with -datacarriersize=0.")
self.test_null_data_transaction(node=self.nodes[1], datas=[b''], success=False)

self.log.info("Testing a null data transaction with a size larger than accepted by -datacarriersize.")
self.test_null_data_transaction(node=self.nodes[2], data=default_size_data, success=False)
self.test_null_data_transaction(node=self.nodes[2], datas=[too_large_data], success=False)

self.log.info("Testing a null data transaction with a size exactly equal to accepted by -datacarriersize.")
self.test_null_data_transaction(node=self.nodes[2], datas=[exact_size_data], success=True)

self.log.info("Testing a null data transaction with a size smaller than accepted by -datacarriersize.")
self.test_null_data_transaction(node=self.nodes[2], data=small_data, success=True)
self.test_null_data_transaction(node=self.nodes[2], datas=[small_data], success=True)

self.log.info("Testing that a single and multiple bare OP_RETURN is always accepted")
for n in (1, 2):
for node in self.nodes:
self.test_bare_op_return(node=node, n=n, success=True)

if __name__ == '__main__':
DataCarrierTest().main()
3 changes: 0 additions & 3 deletions test/functional/test_framework/messages.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,9 +68,6 @@
DEFAULT_ANCESTOR_LIMIT = 25 # default max number of in-mempool ancestors
DEFAULT_DESCENDANT_LIMIT = 25 # default max number of in-mempool descendants

# Default setting for -datacarriersize. 80 bytes of data, +1 for OP_RETURN, +2 for the pushdata opcodes.
MAX_OP_RETURN_RELAY = 83

DEFAULT_MEMPOOL_EXPIRY_HOURS = 336 # hours

def sha256(s):
Expand Down