-limitdummyscriptdatasize option
#29520
Same as #28408, but with one new options:
-limitdummyscriptdatasize which allows you to choose the maximum size of the relayed dummy script with the default value MAX_BLOCK_WEIGHT (maximum size of a block)
PR based on the work of @luke-jr and @LarryRuane with the help of @nsvrn and @ChrisMartl
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept NACK | russeree, glozow, michaelfolkson, 1440000bytes |
| Concept ACK | ChrisMartl, GregTonoski |
| Stale ACK | nsvrn |
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:
-datacarrier option by vostrnad)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.
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.
Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
Leave a comment here, if you need help tracking down a confusing failure.
NACK, engaging in filtering of specific patterns of script is a long term drain on developers and would not effectively curb the problem because of the fact that miners could always just take the transactions in directly, F2Pool, Marathon Slipstream, Luxor, etc …
Relay policy is largely designed to prevent attack vectors and DoS attacks on node runners. Relay policy as a governance mechanism IMO is an incredibly dangerous path to take.
A better approach would be to consider trying to change the consensus rules of the network to better shape long term user behavior. Though this is a harder ask because it would require consensus.
governance mechanism
Please elaborate or provide rationale for this terminology on your comment / review.
long term drain on developers
No true. Some developer contribute and some maintainer “click” on “merge” (effort close to zero).
The “long term drain” is on side of actors attempting to DoS the system.
governance mechanism
Please elaborate or provide rationale for this terminology on your comment / review.
Rules put into place in order to curb specific consensus valid behaviors. An example would be place a restriction on the relaying of a specific amount of sats or even TXIDs that have been ground to form vanity addresses. This would be governance.
NACK, engaging in filtering of specific patterns of script is a long term drain on developers and would not effectively curb the problem because of the fact that miners could always just take the transactions in directly, F2Pool, Marathon Slipstream, Luxor, etc …
They are already doing it so there is no reason to refuse this change. In addition, it is activated by default so it does not change the default relay policy, it only gives the choice to the node runners.
This is largely a duplicate of #28408 that does not address its problems, so my NACK there applies here #28408 (comment)
Also, please take high-level discussion to a different forum such as Delving.
tACK ccc7df4b4903908855ab0db740b9ce59f711aded
relayinscription and relayinscriptionsize with different values on local regtest against inscription/ord spam transactions, works as intended. Error on rejection:0RPC error response: RpcError { code: -26, message: "txn-inscription-exceeded", data: None }
@glozow the comment you linked is extremely long… would you mind summarizing what you see as the outstanding issues so we can get them addressed?
Do you just mean your comments here?
This will not prevent ordinals. Given the high fees, this policy is incentive-incompatible to adopt as a miner. Even if this relay policy is adopted, this has not stopped ordinals in the past (4MB tx in mempool.space, in ordiscan, as tweeted). This change is likely harmful to the node by making its mempool exclude transactions that will be mined. This is a restriction to default policy, which can impact many users beyond an individual node operator, including making it difficult for people to access existing funds, and disrupting compact block relay. This will not prevent data-carrying transactions. See above. It may be best to leave the methods that are most efficient wrt network resource costs. People are calling this “spam” because they don’t like ordinals. While I also personally think Bitcoin is best used for financial transactions, I disagree with this framing of “spam.” In transaction validation and relay logic, we should define spam on the basis of network resources consumed and whether those costs are well bounded and/or paid for, not on the basis of use case. We have no way of detecting whether a bunch of bytes are real/legitimate/useful and, even if we could, it is not protocol development’s place to decide which use cases of Bitcoin are legitimate and which ones aren’t.
544@@ -545,9 +545,10 @@ class CScript : public CScriptBase
545 bool HasValidOps() const;
546
547 /**
548- * Returns whether the script is guaranteed to fail at execution,
549+ * Returns whether the scriptPubKey is guaranteed to fail at execution,
550 * regardless of the initial stack. This allows outputs to be pruned
551- * instantly when entering the UTXO set.
552+ * instantly when entering the UTXO set. Note that this is incorrect for
553+ * witness scripts, which are not always limited by MAX_SCRIPT_SIZE.
In 7e006d2e6f2d656fa74d76b49716dc622a2f8bfa “doc: script: Document that IsUnspendable is incorrect for some witness scripts”
Witness scripts cannot be scriptPubKeys, so this addition seems to be incorrect with the above change of script to scriptPubKey.
315+ switch (opcode) {
316+ case OP_IF: case OP_NOTIF:
317+ ++inside_noop;
318+ break;
319+ case OP_ENDIF:
320+ if (0 == --inside_noop) {
In 822a1f23cb1dedb2bce496c66964400310e9296c “script: Add CScript::DatacarrierBytes”
It’s not clear to me why inside_noop needs to be a counter that is incremeted under essentially the same conditions as inside_conditional. ISTM this could be condensed and combined with with the above code for inside_conditional, and just record the depth of the conditional that the no-op began.
305@@ -306,3 +306,50 @@ int64_t GetVirtualTransactionInputSize(const CTxIn& txin, int64_t nSigOpCost, un
306 {
307 return GetVirtualTransactionSize(GetTransactionInputWeight(txin), nSigOpCost, bytes_per_sigop);
308 }
309+
310+std::pair<CScript, unsigned int> GetScriptForTransactionInput(CScript prevScript, const CTxIn& txin)
In 78fe7b6cd639b12f42e4af71a7f9ed25f71185e2 “policy: GetScriptForTransactionInput to figure out P2SH, witness, taproot”
nit: variable names in new code should use snake_case.
168@@ -168,4 +169,6 @@ static inline int64_t GetVirtualTransactionInputSize(const CTxIn& tx)
169 return GetVirtualTransactionInputSize(tx, 0, 0);
170 }
171
172+std::pair<CScript, unsigned int> GetScriptForTransactionInput(CScript prevScript, const CTxIn&);
In 78fe7b6cd639b12f42e4af71a7f9ed25f71185e2 “policy: GetScriptForTransactionInput to figure out P2SH, witness, taproot”
Can you add a comment that explains what this function does and what it’s return values are.
303+ if (opcode == OP_IF || opcode == OP_NOTIF) {
304+ ++inside_conditional;
305+ } else if (opcode == OP_ENDIF) {
306+ if (!inside_conditional) return size(); // invalid
307+ --inside_conditional;
308+ } else if (opcode == OP_RETURN && !inside_conditional) {
In e9a207d23b588f686d919aa556cd82a921926114 “Apply -datacarriersize to all datacarrying”
As this function is also called on scriptPubKeys, the check for OP_RETURN data outputs is repeated here. However, this means that if -relayinscriptionsize is smaller than -datacarriersize, such OP_RETURN outputs would also be excluded. These is confusing and unexpected behavior as presumably the node operator wished to allow OP_RETURNs but not inscriptions in that situation.
737@@ -738,7 +738,7 @@ def spenders_taproot_active():
738 scripts = [
739 ("pk_codesep", CScript(random_checksig_style(pubs[1]) + bytes([OP_CODESEPARATOR]))), # codesep after checksig
740 ("codesep_pk", CScript(bytes([OP_CODESEPARATOR]) + random_checksig_style(pubs[1]))), # codesep before checksig
741- ("branched_codesep", CScript([random.randbytes(random.randrange(2, 511)), OP_DROP, OP_IF, OP_CODESEPARATOR, pubs[0], OP_ELSE, OP_CODESEPARATOR, pubs[1], OP_ENDIF, OP_CHECKSIG])), # branch dependent codesep
742+ ("branched_codesep", CScript([random.randbytes(random.randrange(2, 75)), OP_DROP, OP_IF, OP_CODESEPARATOR, pubs[0], OP_ELSE, OP_CODESEPARATOR, pubs[1], OP_ENDIF, OP_CHECKSIG])), # branch dependent codesep
In e9a207d23b588f686d919aa556cd82a921926114 “Apply -datacarriersize to all datacarrying”
Can you explain why this test is being changed? I would expect either it doesn’t need to be changed, or it would have more dramatic changes than just a reduction in the stack item’s size.
1468@@ -1468,6 +1469,261 @@ BOOST_AUTO_TEST_CASE(script_HasValidOps)
1469 BOOST_CHECK(!script.HasValidOps());
1470 }
1471
1472+BOOST_AUTO_TEST_CASE(script_DataCarrierBytes)
In 7ddcbd8733348f59ba84bc8312ddf71bdbc061d7 “QA: script_tests: Check GetScriptForTransactionInput and CScript::DataCarrierBytes”
I think this could use some tests for other existing scripts to check that the inscription matching isn’t also accidentally catching other scripts that are in use today, such as lightning (which uses OP_DROP).
It’d also be nice to have a fuzzer for the matching function. Perhaps one that generated valid miniscript and checked that such scripts aren’t caught?
633@@ -634,6 +634,8 @@ void SetupServerArgs(ArgsManager& argsman)
634 "is of this size or less (default: %u)",
635 MAX_OP_RETURN_RELAY),
636 ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
637+ argsman.AddArg("-relayinscription", strprintf("Relay and mine inscription transactions and remove the fee reduction for them(default: %u)", DEFAULT_RELAY_INSCRIPTION), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
In ccc7df4b4903908855ab0db740b9ce59f711aded “add -relayinscription option”
nit: missing a space in “them(default: %u)”
All of the renaming and moving in the last commit needs to be squashed into the commits that introduced the code. New code should not be introduced only to be renamed or removed in a subsequent commit.
Note that I am only looking at the code itself and not giving any opinion on whether this is or is not a good idea.
328+ data_began = opcode_it;
329+ // Match <data> OP_DROP
330+ } else if (opcode <= OP_PUSHDATA4) {
331+ data_began = opcode_it;
332+ } else if (opcode == OP_DROP && last_opcode <= OP_PUSHDATA4) {
333+ counted += it - data_began;
In 822a1f23cb1dedb2bce496c66964400310e9296c “script: Add CScript::DatacarrierBytes”
Inscriptions does not use OP_DROP, nor any other data carrying protocol that I am aware of. Does this exist only to preempt such protocols?
I want to point out that the matching function is incredibly easy to bypass. The main thing that it is looking for is a script construction of OP_FALSE OP_IF. This is trivially bypassed by moving the OP_FALSE out of the script and onto the stack. As scripts beginning with OP_IF are also scripts that people actually want to use, you cannot just match on the OP_IF. The following diff is a test demonstrating this that fails:
0diff --git a/src/test/script_tests.cpp b/src/test/script_tests.cpp
1index a07a1546a8a..c06dd8f9925 100644
2--- a/src/test/script_tests.cpp
3+++ b/src/test/script_tests.cpp
4@@ -1645,6 +1645,22 @@ BOOST_AUTO_TEST_CASE(script_GetScriptForTransactionInput)
5 BOOST_CHECK_EQUAL(scale, 1);
6 BOOST_CHECK_EQUAL(ret_script.InscriptionBytes(), 0);
7 }
8+ { // P2TR scriptpath
9+ CScript prev_script; // scriptPubKey
10+ CTxIn tx_in;
11+ prev_script = CScript() << OP_1 << zeros(32);
12+ // segwit: empty scriptsig
13+ tx_in.scriptSig = CScript();
14+ tx_in.scriptWitness.stack.push_back({});
15+ CScript script = CScript() << OP_IF << OP_8 << OP_9 << OP_11 << OP_ENDIF;
16+ auto script_vec{std::vector<unsigned char>(script.begin(), script.end())};
17+ tx_in.scriptWitness.stack.push_back(script_vec);
18+ tx_in.scriptWitness.stack.push_back(zeros(33)); // control block
19+ auto [ret_script, scale] = GetScriptForTransactionInput(prev_script, tx_in);
20+ BOOST_CHECK(ret_script == script);
21+ BOOST_CHECK_EQUAL(scale, 1);
22+ BOOST_CHECK_EQUAL(ret_script.InscriptionBytes(), 5);
23+ }
24 }
25
26 static CMutableTransaction TxFromHex(const std::string& str)
the last commit needs to be squashed into the commits that introduced the code
done !
ACK - very interesting improvement proposal. Well worth considering.
The option name “-relayinscription” may need renaming. It will not be clearly understood because the “inscription” brand is foreign to Bitcoin and other expoits of the vulnerability of unreachable code (here: OF_FALSE OP_IF) are covered. Perhaps, “-dismiss-bloat-from-mempool” (bounce bloated transactions from mempool) would be more self-explanatory.
Concept NACK
This is very similar in spirit to #28408 and hence my Concept NACK rationale from there applies here too #28408 (comment)
@michaelfolkson There is no reason to refuse this change because it is enabled by default, it does not change the default relay policy.
You refuse the sovereignty of nodes runners and this is unacceptable.
Ocean can move quicker than Core so perhaps Ocean can engage in this game of tit-for-tat or whack-a-mole but Core would look ridiculous if it was to do so. @Retropex: As I allude to in this previous comment I don’t think Core should go down the route of adding new policy options (or changing default policy) every time an inscription protocol is altered in response to policy changes in Core or any other implementation. Core has to make decisions on its default policy and what policy options are available. Other implementations (e.g. Knots) are free to make different decisions. Node runners are free to run a different implementation if their policy preferences aren’t facilitated in Core. The difference between Core and Ocean/Knots is that Core moves slower, would lose this game of whack-a-mole (to the extent that winning is even possible) and would look ridiculous in the process. I don’t think Core should engage in this game regardless of whether we are talking default policy or custom policy options. Having a bunch of pointless policy options 5 years down the line that were only introduced to play this game of whack-a-mole is not a good direction for the project or the software.
That is my view and hence why I’m Concept NACKing this PR and new PRs that crop up with a similar objective. In the spirit of not wanting to create unnecessary noise in this repo I won’t be commenting on this PR again but I will log my Concept NACK on new PRs with a similar objective.
633@@ -634,6 +634,8 @@ void SetupServerArgs(ArgsManager& argsman)
634 "is of this size or less (default: %u)",
635 MAX_OP_RETURN_RELAY),
636 ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
637+ argsman.AddArg("-relayinscription", strprintf("Relay and mine inscription transactions and remove the fee reduction for them (default: %u)", DEFAULT_RELAY_INSCRIPTION), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
638+ argsman.AddArg("-relayinscriptionsize", strprintf("Maximum size of inscription transactions we relay and mine (default: %u)", MAX_INSCRIPTION_RELAY), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
This isn’t just about relaying, it’s about accepting into your mempool at all – if someone sends you the result of an inscribed tx you won’t be able to spend those funds until they confirm, and (obviously) you won’t include them in blocks.
I’d suggest something more like -limitdummyscriptdatasize, which also avoids tying it to one particular protocol; given inscriptions could change their spec to use other formats, and other protocols could adopt a similar format, a more generic name seems better.
-limitdummyscriptdatasize option.
633@@ -634,6 +634,8 @@ void SetupServerArgs(ArgsManager& argsman)
634 "is of this size or less (default: %u)",
635 MAX_OP_RETURN_RELAY),
636 ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
637+ argsman.AddArg("-relayinscription", strprintf("Relay and mine inscription transactions and remove the fee reduction for them (default: %u)", DEFAULT_RELAY_INSCRIPTION), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
MAX_BLOCK_WEIGHT serves to avoid the limit.
836@@ -837,6 +837,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
837 return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, "bad-txns-nonstandard-inputs");
838 }
839
840+ if (InscriptionBytes(tx, m_view) > m_pool.m_max_inscription_bytes.value_or(0)) {
if (m_pool.m_max_inscription_bytes && *m_pool.m_max_inscription_bytes < InscriptionBytes(tx, m_view)) { ... }. That also ensures InscriptionBytes isn’t evaluated at all if it’s not needed.
As this change wouldn’t affect the default behaviour, only make it easier for node runners to opt out of relaying spam (if they perceive inscriptions and arbitrary data of the various kinds we see to be spam of course), surely any objections that core default behaviour shouldn’t be used as a governance mechanism are moot?
Further, if you hold that view, and assert that it is a reason that this pull request shouldn’t be merged, and therefore you seek to restrict node runners optionality when it comes to the data they are willing to relay, isn’t that itself a form of governance? Worse - the default behaviour is then, in fact, being used as governance.
If the Bitcoin network is truly majority in favour of the spam, then nobody will use the feature anyway…
65@@ -65,6 +66,8 @@ static constexpr unsigned int DEFAULT_DESCENDANT_LIMIT{25};
66 static constexpr unsigned int DEFAULT_DESCENDANT_SIZE_LIMIT_KVB{101};
67 /** Default for -datacarrier */
68 static const bool DEFAULT_ACCEPT_DATACARRIER = true;
69+/** Default setting for -limitdummyscriptdatasize */
70+static const unsigned int MAX_DUMMY_SCRIPT_RELAY = MAX_BLOCK_SERIALIZED_SIZE;
0/** The maximum allowed size for a serialized block, in bytes (only for buffer size limits) */
This isn’t a buffer size limit, so this seems like the wrong constant.
MAX_BLOCK_WEIGHT ?
50@@ -51,6 +51,7 @@ struct MemPoolOptions {
51 * If nullopt, any size is nonstandard.
52 */
53 std::optional<unsigned> max_datacarrier_bytes{DEFAULT_ACCEPT_DATACARRIER ? std::optional{MAX_OP_RETURN_RELAY} : std::nullopt};
54+ std::optional<unsigned> max_dummy_script_bytes{std::optional{MAX_DUMMY_SCRIPT_RELAY}};
optional.
633@@ -634,6 +634,7 @@ void SetupServerArgs(ArgsManager& argsman)
634 "is of this size or less (default: %u)",
635 MAX_OP_RETURN_RELAY),
636 ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
637+ argsman.AddArg("-limitdummyscriptdatasize", strprintf("Maximum size of dummy script we relay and mine (default: %u)", MAX_DUMMY_SCRIPT_RELAY), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
836@@ -837,6 +837,10 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
837 return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, "bad-txns-nonstandard-inputs");
838 }
839
840+ if (DummyScriptBytes(tx, m_view) > m_pool.m_max_dummy_script_bytes.value_or(0)) {
tx.GetTotalSize() <= m_max_dummy_script_bytes, perhaps.
tx.GetTotalSize() <= m_max_dummy_script_bytes , the DummyScriptBytes() function will not be called in the rest of the code, so how can it detect inscriptions (or other arbitrary data)?
I just mean:
0if (GetTotalSize > max_dummy_script_bytes) {
1 if (DummyScriptBytes() > max_dummy_script_bytes) { ... }
2}
836@@ -837,6 +837,12 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
837 return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, "bad-txns-nonstandard-inputs");
838 }
839
840+ if (tx.GetTotalSize() > m_pool.m_max_dummy_script_bytes) {
m_max_dummy_script_bytes is still defined as optional<unsigned> ; should presumably just be unsigned, but if not, shouldn’t be compared to things without first checking it’s not nullopt
1043@@ -1044,7 +1044,7 @@ def big_spend_inputs(ctx):
1044 # Test that an input stack size of 1000 elements is permitted, but 1001 isn't.
1045 add_spender(spenders, "tapscript/1000inputs", leaf="t23", **common, inputs=[getter("sign")] + [b'' for _ in range(999)], failure={"leaf": "t24", "inputs": [getter("sign")] + [b'' for _ in range(1000)]}, **ERR_STACK_SIZE)
1046 # Test that pushing a MAX_SCRIPT_ELEMENT_SIZE byte stack element is valid, but one longer is not.
1047- add_spender(spenders, "tapscript/pushmaxlimit", leaf="t25", **common, **SINGLE_SIG, failure={"leaf": "t26"}, **ERR_PUSH_LIMIT)
1048+ add_spender(spenders, "tapscript/pushmaxlimit", standard=False, leaf="t25", **common, **SINGLE_SIG, failure={"leaf": "t26"}, **ERR_PUSH_LIMIT)
I think this change is what’s causing CI to fail – nothing in this PR changes what’s standard by default, so when this testcase turns out to not be rejected from the mempool, it reports it as an error. Changing this to False would only be correct if you were also adding -limitdummyscriptdatasize=80or similar toextra_args`.
I don’t think you should be changing this test case at all, but rather should be adding a test to mempool_datacarrier.py.
I have undo all changes on feature_taproot.py
Implementing tests are way above want I can do, so it’s up for grab.
553@@ -554,6 +554,8 @@ class CScript : public CScriptBase
554 return (size() > 0 && *begin() == OP_RETURN) || (size() > MAX_SCRIPT_SIZE);
555 }
556
557+ size_t DummyScriptBytes() const;
I don’t think script.h is the right place to put policy code. Anything in this file should be considered consensus-critical. My understanding is that all functions in this class are consensus-critical, and adding a new one that isn’t does not seem ideal. See for example commit 87fe71e1fc810ee120a10063fdd26c3245686d54 for some historical context. It wouldn’t be a great outcome if a future change to this function or functionality caused a consensus change and chain split.
I’d say that pure policy code should purely live in the policy folder.
Implementing tests are way above want I can do, so it’s up for grab. [ref]
I don’t think it makes a lot of sense to add features without basic functional tests demonstrating they work as intended. Perhaps someone reading this is up for creating some?
NACK
This option is useless and adding more “filters” to maintain this will be a waste of time for everyone. There are lot of important and useful things which do not get added in bitcoin core because it will be a maintenance burden. Example: #29134
Users who are willing to experiment with different types of “filters” should use knots as it keeps adding them in every release.
Pull requests such as https://github.com/bitcoinknots/bitcoin/pull/78 makes me believe there will be no end to this, no rationale and it wont stop with this pull request. Use knots at your own risk though because a vulnerability wont be surprising the way things are going and under reviewed.
Closing this as it has not had any activity in a while, and feedback has not been addressed.
Disgraceful.
