add -limitdummyscriptdatasize option

[Retropex]

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

[DrahtBot]

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #29942 (Remove redundant -datacarrier option by vostrnad)
• #29309 (Add a -permitbarepubkey option by vostrnad)
• #29086 (refactor: Simply include CTxMemPool::Options in CTxMemPool directly rather than duplicating definition by luke-jr)

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.

[DrahtBot]

🚧 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.

_{Debug: https://github.com/bitcoin/bitcoin/runs/22147391033}

[russeree]

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.

[ChrisMartl]

ACK

[ChrisMartl]

governance mechanism

Please elaborate or provide rationale for this terminology on your comment / review.

[ChrisMartl]

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.

[russeree]

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.

[Retropex]

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.

[glozow]

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.

[nsvrn]

tACK ccc7df4

• Unit and functional tests pass on my local Linux environment
• Tested both relayinscription and relayinscriptionsize with different values on local regtest against inscription/ord spam transactions, works as intended. Error on rejection:

RPC error response: RpcError { code: -26, message: "txn-inscription-exceeded", data: None }

• I'll be precise on "higher high level discussion" as the authority people suggested so: it is imminent to give optionality to nodes which prefer to not relay spam, otherwise they scramble to using/trusting inconvenient patches and community stores of packaged software.

[chrisguida]

@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.

[achow101]

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.

[achow101]

In 822a1f2 "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.

[achow101]

In 78fe7b6 "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.

[achow101]

In 7ddcbd8 "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?

[achow101]

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:

diff --git a/src/test/script_tests.cpp b/src/test/script_tests.cpp
index a07a1546a8a..c06dd8f9925 100644
--- a/src/test/script_tests.cpp
+++ b/src/test/script_tests.cpp
@@ -1645,6 +1645,22 @@ BOOST_AUTO_TEST_CASE(script_GetScriptForTransactionInput)
         BOOST_CHECK_EQUAL(scale, 1);
         BOOST_CHECK_EQUAL(ret_script.InscriptionBytes(), 0);
     }
+    { // P2TR scriptpath
+        CScript prev_script; // scriptPubKey
+        CTxIn tx_in;
+        prev_script = CScript() << OP_1 << zeros(32);
+        // segwit: empty scriptsig
+        tx_in.scriptSig = CScript();
+        tx_in.scriptWitness.stack.push_back({});
+        CScript script = CScript() << OP_IF << OP_8 << OP_9 << OP_11 << OP_ENDIF;
+        auto script_vec{std::vector<unsigned char>(script.begin(), script.end())};
+        tx_in.scriptWitness.stack.push_back(script_vec);
+        tx_in.scriptWitness.stack.push_back(zeros(33)); // control block
+        auto [ret_script, scale] = GetScriptForTransactionInput(prev_script, tx_in);
+        BOOST_CHECK(ret_script == script);
+        BOOST_CHECK_EQUAL(scale, 1);
+        BOOST_CHECK_EQUAL(ret_script.InscriptionBytes(), 5);
+    }
 }
 
 static CMutableTransaction TxFromHex(const std::string& str)

[Retropex]

the last commit needs to be squashed into the commits that introduced the code

done !

[GregTonoski]

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.

[michaelfolkson]

Concept NACK

This is very similar in spirit to #28408 and hence my Concept NACK rationale from there applies here too #28408 (comment)

[Retropex]

@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.

[michaelfolkson]

@Retropex:

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.

[Bitcoin-Lebowski]

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...

[maflcko]

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 87fe71e 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.

[ajtowns]

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?

[1440000bytes]

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 bitcoinknots#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.

[DrahtBot]

🐙 This pull request conflicts with the target branch and needs rebase.