From 9f36962b07eff2369577a17c8adeaa0433697e1c Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Fri, 2 May 2025 10:01:13 -0400 Subject: [PATCH] policy: uncap datacarrier by default Datacarrier output script sizes and output counts are now uncapped by default. To avoid introducing another startup argument, we modify the OP_RETURN accounting to "budget" the spk sizes. If a user has set a custom default, this results in that budget being spent over the sum of all OP_RETURN outputs' scripts in the transaction, no longer capping the number of OP_RETURN outputs themselves. This should allow a superset of current behavior while respecting the passed argument in terms of total arbitrary data storage. Co-authored-by: Anthony Towns --- src/init.cpp | 4 +-- src/policy/policy.cpp | 27 +++++++--------- src/policy/policy.h | 7 ++-- src/test/fuzz/key.cpp | 4 +-- src/test/fuzz/script.cpp | 2 +- src/test/multisig_tests.cpp | 2 +- src/test/transaction_tests.cpp | 35 +++++++++++--------- test/functional/mempool_accept.py | 7 ---- test/functional/mempool_datacarrier.py | 37 ++++++++++++---------- test/functional/test_framework/messages.py | 6 ++-- 10 files changed, 64 insertions(+), 67 deletions(-) diff --git a/src/init.cpp b/src/init.cpp index bfb42aaa3b6..629d612abd2 100644 --- a/src/init.cpp +++ b/src/init.cpp @@ -632,8 +632,8 @@ void SetupServerArgs(ArgsManager& argsman, bool can_listen_ipc) 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); argsman.AddArg("-datacarriersize", - strprintf("Relay and mine transactions whose data-carrying raw scriptPubKey " - "is of this size or less (default: %u)", + strprintf("Relay and mine transactions whose data-carrying raw scriptPubKeys in aggregate " + "are of this size or less, allowing multiple outputs (default: %u)", MAX_OP_RETURN_RELAY), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY); argsman.AddArg("-permitbaremultisig", strprintf("Relay transactions creating non-P2SH multisig outputs (default: %u)", DEFAULT_PERMIT_BAREMULTISIG), ArgsManager::ALLOW_ANY, diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp index 32a31ea653c..fdc2fdb9cdf 100644 --- a/src/policy/policy.cpp +++ b/src/policy/policy.cpp @@ -76,7 +76,7 @@ std::vector GetDust(const CTransaction& tx, CFeeRate dust_relay_rate) return dust_outputs; } -bool IsStandard(const CScript& scriptPubKey, const std::optional& max_datacarrier_bytes, TxoutType& whichType) +bool IsStandard(const CScript& scriptPubKey, TxoutType& whichType) { std::vector > vSolutions; whichType = Solver(scriptPubKey, vSolutions); @@ -91,10 +91,6 @@ bool IsStandard(const CScript& scriptPubKey, const std::optional& max_ return false; if (m < 1 || m > n) return false; - } else if (whichType == TxoutType::NULL_DATA) { - if (!max_datacarrier_bytes || scriptPubKey.size() > *max_datacarrier_bytes) { - return false; - } } return true; @@ -137,17 +133,22 @@ bool IsStandardTx(const CTransaction& tx, const std::optional& max_dat } } - unsigned int nDataOut = 0; + unsigned int datacarrier_bytes_left = max_datacarrier_bytes.value_or(0); TxoutType whichType; for (const CTxOut& txout : tx.vout) { - if (!::IsStandard(txout.scriptPubKey, max_datacarrier_bytes, whichType)) { + if (!::IsStandard(txout.scriptPubKey, whichType)) { reason = "scriptpubkey"; return false; } - if (whichType == TxoutType::NULL_DATA) - nDataOut++; - else if ((whichType == TxoutType::MULTISIG) && (!permit_bare_multisig)) { + if (whichType == TxoutType::NULL_DATA) { + unsigned int size = txout.scriptPubKey.size(); + if (size > datacarrier_bytes_left) { + reason = "datacarrier"; + return false; + } + datacarrier_bytes_left -= size; + } else if ((whichType == TxoutType::MULTISIG) && (!permit_bare_multisig)) { reason = "bare-multisig"; return false; } @@ -159,12 +160,6 @@ bool IsStandardTx(const CTransaction& tx, const std::optional& max_dat return false; } - // only one OP_RETURN txout is permitted - if (nDataOut > 1) { - reason = "multi-op-return"; - return false; - } - return true; } diff --git a/src/policy/policy.h b/src/policy/policy.h index 2151ec13dd0..e62efa02e3e 100644 --- a/src/policy/policy.h +++ b/src/policy/policy.h @@ -73,10 +73,9 @@ static constexpr unsigned int DEFAULT_DESCENDANT_SIZE_LIMIT_KVB{101}; /** Default for -datacarrier */ static const bool DEFAULT_ACCEPT_DATACARRIER = true; /** - * Default setting for -datacarriersize. 80 bytes of data, +1 for OP_RETURN, - * +2 for the pushdata opcodes. + * Default setting for -datacarriersize in vbytes. */ -static const unsigned int MAX_OP_RETURN_RELAY = 83; +static const unsigned int MAX_OP_RETURN_RELAY = MAX_STANDARD_TX_WEIGHT / WITNESS_SCALE_FACTOR; /** * An extra transaction can be added to a package, as long as it only has one * ancestor and is no larger than this. Not really any reason to make this @@ -136,7 +135,7 @@ CAmount GetDustThreshold(const CTxOut& txout, const CFeeRate& dustRelayFee); bool IsDust(const CTxOut& txout, const CFeeRate& dustRelayFee); -bool IsStandard(const CScript& scriptPubKey, const std::optional& max_datacarrier_bytes, TxoutType& whichType); +bool IsStandard(const CScript& scriptPubKey, TxoutType& whichType); /** Get the vout index numbers of all dust outputs */ std::vector GetDust(const CTransaction& tx, CFeeRate dust_relay_rate); diff --git a/src/test/fuzz/key.cpp b/src/test/fuzz/key.cpp index d851e33adc5..e4bedff8b51 100644 --- a/src/test/fuzz/key.cpp +++ b/src/test/fuzz/key.cpp @@ -150,12 +150,12 @@ FUZZ_TARGET(key, .init = initialize_key) assert(fillable_signing_provider_pub.HaveKey(pubkey.GetID())); TxoutType which_type_tx_pubkey; - const bool is_standard_tx_pubkey = IsStandard(tx_pubkey_script, std::nullopt, which_type_tx_pubkey); + const bool is_standard_tx_pubkey = IsStandard(tx_pubkey_script, which_type_tx_pubkey); assert(is_standard_tx_pubkey); assert(which_type_tx_pubkey == TxoutType::PUBKEY); TxoutType which_type_tx_multisig; - const bool is_standard_tx_multisig = IsStandard(tx_multisig_script, std::nullopt, which_type_tx_multisig); + const bool is_standard_tx_multisig = IsStandard(tx_multisig_script, which_type_tx_multisig); assert(is_standard_tx_multisig); assert(which_type_tx_multisig == TxoutType::MULTISIG); diff --git a/src/test/fuzz/script.cpp b/src/test/fuzz/script.cpp index 07a49e039fb..9aa50559cbd 100644 --- a/src/test/fuzz/script.cpp +++ b/src/test/fuzz/script.cpp @@ -53,7 +53,7 @@ FUZZ_TARGET(script, .init = initialize_script) } TxoutType which_type; - bool is_standard_ret = IsStandard(script, std::nullopt, which_type); + bool is_standard_ret = IsStandard(script, which_type); if (!is_standard_ret) { assert(which_type == TxoutType::NONSTANDARD || which_type == TxoutType::NULL_DATA || diff --git a/src/test/multisig_tests.cpp b/src/test/multisig_tests.cpp index 29a73d03d25..b996f3cc061 100644 --- a/src/test/multisig_tests.cpp +++ b/src/test/multisig_tests.cpp @@ -144,7 +144,7 @@ BOOST_AUTO_TEST_CASE(multisig_IsStandard) const auto is_standard{[](const CScript& spk) { TxoutType type; - bool res{::IsStandard(spk, std::nullopt, type)}; + bool res{::IsStandard(spk, type)}; if (res) { BOOST_CHECK_EQUAL(type, TxoutType::MULTISIG); } diff --git a/src/test/transaction_tests.cpp b/src/test/transaction_tests.cpp index 7a60ea25d3f..54b3216a4c6 100644 --- a/src/test/transaction_tests.cpp +++ b/src/test/transaction_tests.cpp @@ -797,14 +797,14 @@ BOOST_AUTO_TEST_CASE(test_IsStandard) CKey key = GenerateRandomKey(); t.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key.GetPubKey())); - constexpr auto CheckIsStandard = [](const auto& t) { + constexpr auto CheckIsStandard = [](const auto& t, const unsigned int max_op_return_relay = MAX_OP_RETURN_RELAY) { std::string reason; - BOOST_CHECK(IsStandardTx(CTransaction{t}, MAX_OP_RETURN_RELAY, g_bare_multi, g_dust, reason)); + BOOST_CHECK(IsStandardTx(CTransaction{t}, max_op_return_relay, g_bare_multi, g_dust, reason)); BOOST_CHECK(reason.empty()); }; - constexpr auto CheckIsNotStandard = [](const auto& t, const std::string& reason_in) { + constexpr auto CheckIsNotStandard = [](const auto& t, const std::string& reason_in, const unsigned int max_op_return_relay = MAX_OP_RETURN_RELAY) { std::string reason; - BOOST_CHECK(!IsStandardTx(CTransaction{t}, MAX_OP_RETURN_RELAY, g_bare_multi, g_dust, reason)); + BOOST_CHECK(!IsStandardTx(CTransaction{t}, max_op_return_relay, g_bare_multi, g_dust, reason)); BOOST_CHECK_EQUAL(reason_in, reason); }; @@ -858,15 +858,13 @@ 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) + // Custom 83-byte TxoutType::NULL_DATA (standard with max_op_return_relay of 83) t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3804678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex; - BOOST_CHECK_EQUAL(MAX_OP_RETURN_RELAY, t.vout[0].scriptPubKey.size()); - CheckIsStandard(t); + BOOST_CHECK_EQUAL(83, t.vout[0].scriptPubKey.size()); + CheckIsStandard(t, /*max_op_return_relay=*/83); - // MAX_OP_RETURN_RELAY+1-byte TxoutType::NULL_DATA (non-standard) - t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3804678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3800"_hex; - BOOST_CHECK_EQUAL(MAX_OP_RETURN_RELAY + 1, t.vout[0].scriptPubKey.size()); - CheckIsNotStandard(t, "scriptpubkey"); + // Non-standard if max_op_return_relay datacarrier arg is one less + CheckIsNotStandard(t, "datacarrier", /*max_op_return_relay=*/82); // Data payload can be encoded in any way... t.vout[0].scriptPubKey = CScript() << OP_RETURN << ""_hex; @@ -888,21 +886,28 @@ BOOST_AUTO_TEST_CASE(test_IsStandard) t.vout[0].scriptPubKey = CScript() << OP_RETURN; CheckIsStandard(t); - // Only one TxoutType::NULL_DATA permitted in all cases + // Multiple TxoutType::NULL_DATA are permitted t.vout.resize(2); t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex; t.vout[0].nValue = 0; t.vout[1].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex; t.vout[1].nValue = 0; - CheckIsNotStandard(t, "multi-op-return"); + CheckIsStandard(t); t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex; 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); + + t.vout[0].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3804678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex; + t.vout[1].scriptPubKey = CScript() << OP_RETURN << "04678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef3804678afdb0fe5548271967f1a67130b7105cd6a828e03909a67962e0ea1f61deb649f6bc3f4cef38"_hex; + const auto datacarrier_size = t.vout[0].scriptPubKey.size() + t.vout[1].scriptPubKey.size(); + CheckIsStandard(t); // Default max relay should never trigger + CheckIsStandard(t, /*max_op_return_relay=*/datacarrier_size); + CheckIsNotStandard(t, "datacarrier", /*max_op_return_relay=*/datacarrier_size-1); // Check large scriptSig (non-standard if size is >1650 bytes) t.vout.resize(1); diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py index 9014ab260e1..f4b21cd2044 100755 --- a/test/functional/mempool_accept.py +++ b/test/functional/mempool_accept.py @@ -326,13 +326,6 @@ class MempoolAcceptanceTest(BitcoinTestFramework): result_expected=[{'txid': tx.rehash(), 'allowed': False, 'reject-reason': 'dust'}], rawtxs=[tx.serialize().hex()], ) - tx = tx_from_hex(raw_tx_reference) - tx.vout[0].scriptPubKey = CScript([OP_RETURN, b'\xff']) - tx.vout = [tx.vout[0]] * 2 - self.check_mempool_result( - result_expected=[{'txid': tx.rehash(), 'allowed': False, 'reject-reason': 'multi-op-return'}], - rawtxs=[tx.serialize().hex()], - ) self.log.info('A timelocked transaction') tx = tx_from_hex(raw_tx_reference) diff --git a/test/functional/mempool_datacarrier.py b/test/functional/mempool_datacarrier.py index 48e636caa22..0613b52b491 100755 --- a/test/functional/mempool_datacarrier.py +++ b/test/functional/mempool_datacarrier.py @@ -18,14 +18,16 @@ from test_framework.wallet import MiniWallet from random import randbytes +# The historical maximum, now used to test coverage +CUSTOM_DATACARRIER_ARG = 83 class DataCarrierTest(BitcoinTestFramework): def set_test_params(self): self.num_nodes = 4 self.extra_args = [ - [], - ["-datacarrier=0"], - ["-datacarrier=1", f"-datacarriersize={MAX_OP_RETURN_RELAY - 1}"], + [], # default is uncapped + ["-datacarrier=0"], # no relay of datacarrier + ["-datacarrier=1", f"-datacarriersize={CUSTOM_DATACARRIER_ARG}"], ["-datacarrier=1", "-datacarriersize=2"], ] @@ -41,32 +43,33 @@ class DataCarrierTest(BitcoinTestFramework): 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) + assert_raises_rpc_error(-26, "datacarrier", self.wallet.sendrawtransaction, from_node=node, tx_hex=tx_hex) 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 = randbytes(MAX_OP_RETURN_RELAY - 3) - too_long_data = randbytes(MAX_OP_RETURN_RELAY - 2) - small_data = randbytes(MAX_OP_RETURN_RELAY - 4) + # By default, any size is allowed. + + # If it is custom set to 83, the historical value, + # only 80 bytes are used for data (+1 for OP_RETURN, +2 for the pushdata opcodes). + custom_size_data = randbytes(CUSTOM_DATACARRIER_ARG - 3) + too_long_data = randbytes(CUSTOM_DATACARRIER_ARG - 2) + extremely_long_data = randbytes(MAX_OP_RETURN_RELAY - 200) one_byte = randbytes(1) zero_bytes = randbytes(0) - 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 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 a null data transaction succeeds for default arg regardless of size.") + self.test_null_data_transaction(node=self.nodes[0], data=too_long_data, success=True) + self.test_null_data_transaction(node=self.nodes[0], data=extremely_long_data, 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.test_null_data_transaction(node=self.nodes[1], data=custom_size_data, 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], data=too_long_data, success=False) - 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.log.info("Testing a null data transaction with a size equal to -datacarriersize.") + self.test_null_data_transaction(node=self.nodes[2], data=custom_size_data, success=True) self.log.info("Testing a null data transaction with no data.") self.test_null_data_transaction(node=self.nodes[0], data=None, success=True) diff --git a/test/functional/test_framework/messages.py b/test/functional/test_framework/messages.py index e6f72f43b54..5549d8f753c 100755 --- a/test/functional/test_framework/messages.py +++ b/test/functional/test_framework/messages.py @@ -73,8 +73,10 @@ WITNESS_SCALE_FACTOR = 4 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 setting for -datacarriersize. +MAX_OP_RETURN_RELAY = 100_000 + DEFAULT_MEMPOOL_EXPIRY_HOURS = 336 # hours