Tapscript is missing the policy size limit check. The limit is inconsistent between witness and scriptSig, so for now this uses the (higher) witness size limit.
We should probably unify these on a single limit size (and make it configurable?), but for now this just addresses the oversight the same way it is for the other checks.
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 | vostrnad, Psifour, reardencode, russeree, petertodd, 1440000bytes, sipa |
| Concept ACK | ChrisMartl, Retropex, nsvrn |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
ACK
This policy should have been added with the SegWit V1 deployment.
What is the goal of this pull request?
ACK, don’t know why this has not been implemented with Taproot.
Why is a limit on script size no longer needed? Since there is no scriptCode directly included in the signature hash (only indirectly through a precomputable tapleaf hash), the CPU time spent on a signature check is no longer proportional to the size of the script being executed.
https://github.com/bitcoin/bips/blob/master/bip-0342.mediawiki#resource-limits
Concept NACK
The reason for the relaxed policy on tapscripts is that OP_CHECKSIGADD multisig is no longer limited to 20 public keys like OP_CHECKMULTISIG, and is only implicitly bound by the block weight limit (and max tx weight by standardness rules). This has been discussed during the development of Taproot, there is no oversight, and the author knows this because he actively participated in those discussions.
As always, I recommend anyone proposing a change to Bitcoin Core’s default policy to first start a thread on the mailing list.
Concept NACK
The reason for the relaxed policy on tapscripts is that OP_CHECKSIGADD multisig is no longer [limited to 20 public keys like OP_CHECKMULTISIG](https://github.com/bitcoin/bitcoin/blob/61de64df
Maybe the policy can be added back with an account exception for multi-sig limits?
Maybe the policy can be added back with an account exception for multi-sig limits?
I don’t know how you’d ensure that scripts that aren’t pure multisig aren’t affected, for example a multisig + time lock, and not have the policy be trivially bypassable at the same time.
In any case, this PR has the same goal as #28408 (filtering inscriptions) and most NACKs from that PR apply here as well.
Concept NACK
For further context please see (9) of the Rationale section of BIP342 which explains why the threat that this policy rule was created to mitigate doesn’t apply to tapscript.
there is conflict of interests, i guess…
Why is a limit on script size no longer needed? Since there is no scriptCode directly included in the signature hash (only indirectly through a precomputable tapleaf hash), the CPU time spent on a signature check is no longer proportional to the size of the script being executed.
This is clearly not enough to justify an abandonment of limits. It’s as if Google was disabling gmail’s anti-spam filters because the cost of processing an email is marginal on their servers. Makes literally no sense.
This is clearly not enough to justify an abandonment of limits.
This is not ‘abandonment of limits’. The rule was created for a specific situation that presented a specific threat, SegWit (tapscript specifically) is outside the scope of that threat.
For reference as to how absurd this is, it would be like me proposing that we should apply the bare multi-sig limit (a policy rule created to address a specific threat) to all multisigs.
there is conflict of interests, i guess…
Thank you for acknowledging the work I put in. It has been a great experience of onboarding people to Bitcoin and helping hundreds of people get nodes setup.
Concept NACK
This limit was also removed to reduce the complexity of formal verification systems for script. The limit is no longer needed for the reasons already stated.
Concept ACK
The only current use case that is abusing no-limit script is for filling the entire block with non-monetary transactions. The case for adding a limit strengthens further if the only other “future” no-limit use-case argument is coming from those with malicious intent of creating secondary markets for trading dust transactions.
Concept NACK, the implied size of the Taproot witness script is up to 4,000,000 weight units (bytes) via consensus rules. A.k.a up to the size of a block. There also already exists a second policy check on the size of these scripts which is up to 400,000 WU for the maximum transaction weight or 1/10th of a block. As an aside, I will likely be NACKing any PRs that attempt to further tighten relay policy for non CPU based DoS vectors as these endeavors now are causing actual fragmentation a somewhat consistent view across mempools.
In it’s current state this change would make it hard for users to redeem their existing tap scripts that are larger your proposed limit.
In addition to this there are projects such as BitVM that do depend on large tap scripts to secure users funds.
Concept NACK
I won’t add this to Libre Relay. So it’ll be yet another pointless change the profit maximizing miners ignore, rendering Bitcoin Core less useful out of the box.
Since PR author failed to answer the question about goal of this PR: #29769 (comment)
Concept NACK
IIUC primary goal of this pull request is to educate everyone about taproot and some drama
Concept NACK
I won’t add this to Libre Relay. So it’ll be yet another pointless change the profit maximizing miners ignore, rendering Bitcoin Core less useful out of the box.
So you’re implying that the only reasonable limits are consensus changes and that relay level filtering doesn’t matter
So you’re implying that the only reasonable limits are consensus changes and that relay level filtering doesn’t matter
Basically yes. As long as there is non-trivial economic demand for a particular transaction form, trying to filter it will cause problems.
The only reasonable filters for for clear technical reasons. Eg the script version zero DoS attacks, bits reserved for soft-fork upgrades, etc.
Concept NACK. This is not an oversight, and a step backwards.
The reason for having this type of policy rule in earlier Script version environments is to discourage constructs that suffer from DoS concerns in the interpreter (known and unknown). For BIP342 we carefully analyzed any sources of non-linear scaling effects such as the amount of data hashed, and data structure growth:
It is of course possible there are oversights, and those might warrant measures to address them, but absent such concerns, I don’t think this applies.
My belief that most proponents of this change however see this as a way to reduce unwanted on-chain data storage. While there is certainly a ton of data that I personally consider unwanted (and frankly, dumb), I don’t believe this will address that:
In short, my view is that on-chain data storage is inherent to having a flexible scripting system, and ultimately Bitcoin already has a mitigation to bound its impact on validators: the block weight limit.
207@@ -208,6 +208,7 @@ def add_options(self, parser):
208
209 def set_test_params(self):
210 self.num_nodes = 1
211+ self.extra_args = [["-acceptnonstdtxn"]]
Just want to add: even if this change were a good idea, changing the wallet tests to allow non standard txs in the mempool is a terrible idea.
The point of wallet tests is to check that we actually can create and spend payments to addresses we support. If any of those addresses aren’t acceptable to our own mempool by default, that’s a bug, and it shouldn’t be hidden in this way. If we’re deliberately making some addresses unspendable with a default config despite having previously supported them, that should be addressed by explicitly removing those classes of addresses from the test cases.
Many people here conflating consensus (as modified by BIPs) with spam filters/policy. The reason why it’s reasonable to relax consensus rules is in a large part because spam is mitigated by policy. If policy is to be discarded, we will need to impose all these limits strictly in consensus - that has not been proposed, and as such, reasonable policies must be maintained until it is. You can’t just throw out an important security check without replacing it by something else!
Taproot’s purpose was to make it so scripts could be omitted on-chain entirely (all parties verify the script passes, and sign the keypath instead), not to make for unlimited on-chain spamming.
I will likely be NACKing any PRs that attempt to further tighten relay policy for non CPU based DoS vectors as these endeavors now are causing actual fragmentation a somewhat consistent view across mempools. @russeree Mempools are not SUPPOSED to be consistent in the first place.
This is not an oversight, and a step backwards.
If it is not an oversight, then you’re saying it was intentionally snuck in behind the Bitcoin community’s backs. There was no discussion of removing spam filters, and no consensus to do so. The very idea is absurd and would never have gained support. @glozow Why did you abuse your repo access to inappropriately close this PR?
The decision to close this PR was primarily based on the well-reasoned NACKs. Based on the technical arguments given by you and the reviewers, it is clear that the PR isn’t a good idea or is at least very controversial. Changes to transaction relay policy that can impact people beyond the node operator should be discussed in the broader community beyond just Bitcoin Core. For example, I’d recommend starting a thread on the mailing list or Delving Bitcoin.
If you disagree and believe it was closed unfairly, I think it would be appropriate to bring this up to the larger group during the weekly irc meeting. I would be happy to re-open this PR if it seems that actually this has strong technical motivation, is more accepted by the community, and/or is determined to have been closed unfairly.
You are asking the wrong question, instead it should be framed as “what technical threat is posed and is said threat sufficient to warrant the creation of a new standardness rule?” The answers to those questions are “none” and subsequently “no”.
Applying rules meant to solve a specific problem to systems that can’t/don’t exhibit the same vulnerabilities would be wildly inappropriate. It would be similar to applying the bare multi-sig limit (3) to ALL multisigs.
TL;DR: The PR attempts to solve a non-issue with sub-par engineering. Luckily the standard for core is slightly hire than “isn’t a breaking change”.
… policy rule in earlier Script version environments is to discourage constructs that suffer from DoS concerns in the interpreter (known and unknown) (…) It is of course possible there are oversights…
Can I ask to point to test results of the change introduced in TapRoot (relaxation of the 3600 bytes script size standard limit/mempool policy rule which is not described in BIPs), please?