Policy: Enforce witness script size limit for tapscript #29769

pull luke-jr wants to merge 1 commits into bitcoin:master from luke-jr:policy_tapscript_size changing 5 files +9 −3
  1. luke-jr commented at 7:41 pm on March 30, 2024: member

    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.

  2. DrahtBot commented at 7:41 pm on March 30, 2024: contributor

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

  3. DrahtBot added the label TX fees and policy on Mar 30, 2024
  4. ChrisMartl commented at 7:58 pm on March 30, 2024: none

    ACK

    This policy should have been added with the SegWit V1 deployment.

  5. Retropex approved
  6. Retropex commented at 8:19 pm on March 30, 2024: none
    ACK, don’t know why this has not been implemented with Taproot.
  7. DrahtBot added the label CI failed on Mar 30, 2024
  8. Policy: Enforce witness script size limit for tapscript f612a52b91
  9. luke-jr force-pushed on Mar 31, 2024
  10. 1440000bytes commented at 8:54 am on March 31, 2024: none

    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

  11. vostrnad commented at 9:00 am on March 31, 2024: none

    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.

  12. earonesty commented at 12:16 pm on March 31, 2024: none

    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?

  13. vostrnad commented at 1:06 pm on March 31, 2024: none

    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.

  14. Psifour commented at 1:16 pm on March 31, 2024: none

    Concept NACK

    • attempt to rehash #28408 in different clothing
    • lacks context and/or fails to address why we should discard the previously agreed upon decision to NOT do this (we all forget past conversations at times so it is completely understandable)
    • attempts to expand an existing policy rule to cover a case where there is no technical reason to inhibit users

    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.

  15. Retropex commented at 1:32 pm on March 31, 2024: none

    there is conflict of interests, i guess…

  16. Retropex commented at 1:36 pm on March 31, 2024: none

    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.

  17. Psifour commented at 1:55 pm on March 31, 2024: none

    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.

  18. reardencode commented at 3:31 pm on March 31, 2024: none

    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.

  19. rot13maxi commented at 3:41 pm on March 31, 2024: none
    People are trying to build applications and protocols that use large scripts (BitVM which is a generalized optimistic state channel, for example). We’ve had an implicit policy on tapscripts since taproot was activated, which is the block size. In order to keep this from being a breaking change, if you want to add a new policy option, then the default should match current behavior. Users can choose to dial the limit downwards.
  20. nsvrn commented at 6:04 pm on March 31, 2024: contributor

    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.

  21. russeree commented at 8:21 pm on March 31, 2024: contributor

    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.

  22. petertodd commented at 11:12 pm on March 31, 2024: contributor

    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.

  23. 1440000bytes commented at 0:52 am on April 1, 2024: none

    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

  24. earonesty commented at 1:08 pm on April 1, 2024: none

    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

  25. petertodd commented at 1:45 pm on April 1, 2024: contributor

    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.

  26. sipa commented at 2:26 pm on April 1, 2024: member

    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:

    • some were addressed
      • quadratic hashing through an O(1) hashing scheme, see BIP341 fn15
      • execution stack growth which was addressed in #16902
    • others have remaining consensus rules to curb their impact (the 1000 stack element limit remains in BIP342, as e.g. the cost of OP_ROT scales with the stack size).

    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:

    • Given the clear economic demand for this data (for better or worse), the best a policy rule can do is provide a mild inconvenience. The worst it can do is cause the development of mempool-bypassing transaction relay rails to miners, which I consider far more harmful in the long term that any current hype.
    • The specific change here might not even do that, as it’s only a tiny marginal cost to satisfy the policy (split things across multiple inputs, or use other mechanisms).
    • The justification that once existed for data-filtering policies just doesn’t apply anymore. Before organic transaction demand was sufficient to fill blocks, I think it was quite reasonable to discourage applications from freeloading on the unused space, as it actively increased resource costs for fully validating nodes of the nascent ecosystem. Today, unwanted data or not, blocks are full anyway, and the cost to validating nodes in the same.

    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.

  27. glozow closed this on Apr 2, 2024

  28. in test/functional/wallet_miniscript.py:211 in f612a52b91
    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"]]
    


    ajtowns commented at 9:26 am on April 2, 2024:

    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.

  29. luke-jr commented at 7:53 pm on April 20, 2024: member

    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?

  30. glozow commented at 4:04 pm on May 2, 2024: member

    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.

  31. GregTonoski commented at 10:22 am on May 19, 2024: none
    Can I ask @glozow to summarize cons, please? I couldn’t find any in spite of huge amount of text in the comments. The PR does not introduce a “breaking change” contrary to what is claimed by opponents.
  32. Psifour commented at 11:16 am on May 19, 2024: none

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

  33. GregTonoski commented at 1:31 pm on May 19, 2024: none

    … 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?


github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-21 09:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me