policy: Allow non-standard scripts with -acceptnonstdtxn=1 #28334

pull ajtowns wants to merge 2 commits into bitcoin:master from ajtowns:202303-acceptnonstdscript changing 2 files +15 −13
  1. ajtowns commented at 4:30 AM on August 24, 2023: contributor

    This PR changes -acceptnonstdtxn=1 so that it also skips the non-mandatory script checks, allowing txs that (eg) use OP_NOPx or OP_SUCCESS into the mempool. This remains only available on test nets.

  2. DrahtBot commented at 4:30 AM on August 24, 2023: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK darosior, moonsettler, sipa, instagibbs

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #29280 (Implement OP_CHECKTEMPLATEVERIFY by reardencode)

    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.

  3. DrahtBot added the label TX fees and policy on Aug 24, 2023
  4. darosior commented at 7:43 AM on August 24, 2023: member

    Concept ACK. Makes sense to me.

  5. moonsettler commented at 11:08 AM on August 24, 2023: none

    Concept ACK!

  6. ajtowns commented at 11:13 AM on August 24, 2023: contributor

    Added a commit to ensure that the mempool is at least enforcing the script rules that will be required for the next block. Not sure that's sensible: it only makes a difference if there's a rule in GetBlockScriptFlags that's not in MANDATORY_SCRIPT_VERIFY_FLAGS, and if there is such a rule, even with this patch, you could add a tx to the mempool prior to the rule becoming active, in which case it wouldn't magically be removed from the mempool when it became active. There's already a double check in the mining code (test_block_validity) which would prevent PoW being wasted on a block that included an invalid tx because it was somehow made it into the mempool.

  7. sipa commented at 12:41 PM on August 24, 2023: member

    Concept ACK

  8. instagibbs commented at 4:55 PM on August 24, 2023: member

    have -acceptnonstdtxn=1 skip the non-mandatory script checks,

    Can you give a bit of motivation for this second part?

  9. DrahtBot added the label CI failed on Aug 24, 2023
  10. ajtowns force-pushed on Aug 25, 2023
  11. ajtowns commented at 6:26 AM on August 25, 2023: contributor

    have -acceptnonstdtxn=1 skip the non-mandatory script checks,

    Can you give a bit of motivation for this second part?

    Mostly that STANDARD_SCRIPT_VERIFY_FLAGS violations sounds like something that acceptnonstdtxn should allow. But some particular examples:

    • When investigating the btcd bugs that crashed lnd, I tried using an OP_SUCCESS to overcome the standard limits, but was blocked by these checks. IIRC I manually disabled them.
    • There was a discussion about being able to see inquisition txs on signet in mempool.space prior to them being mined, without having to run inquisition node software. -acceptnonstdtxn seemed like a way to make this possible, except it doesn't actually work. https://twitter.com/fiatjaf/status/1632710159163170821
    • It makes it hard to mess around with things on regtest https://twitter.com/4moonsettler/status/1694024772403834963
  12. in src/validation.cpp:2045 in a656ecc246 outdated 
    2041 | @@ -2016,33 +2042,33 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex& block_index, const Ch
2042 |      // mainnet.
2043 |      // For simplicity, always leave P2SH+WITNESS+TAPROOT on except for the two
2044 |      // violating blocks.
2045 | -    uint32_t flags{SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_TAPROOT};
2046 | +    MandatoryFlags flags{MandatoryFlag<SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_TAPROOT>};
    ajtowns commented at 6:34 AM on August 25, 2023:

    Probably this commit should be in its own PR since it touches block consensus code. I just wanted to make sure that there is a path forward to ensuring we don't introduce mempool bugs in future by updating consensus without also updating MANDATORY_SCRIPT_VERIFY_FLAGS. This gives compile time errors:

    validation.cpp:2070:11: error: no viable overloaded '|='
    flags |= SCRIPT_VERIFY_CLEANSTACK;

    or

    validation.cpp:2013:5: error: static_assert failed due to requirement '(16384U & MANDATORY_SCRIPT_VERIFY_FLAGS) == 16384U' "MandatoryFlag not included in MANDATORY_SCRIPT_VERIFY_FLAGS"
....
validation.cpp:2071:14: note: in instantiation of variable template specialization '(anonymous namespace)::MandatoryFlag<16384>' requested here
    flags |= MandatoryFlag<SCRIPT_VERIFY_NULLFAIL>;
  13. in src/validation.cpp:2048 in a656ecc246 outdated 
    2041 | @@ -2016,33 +2042,33 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex& block_index, const Ch
2042 |      // mainnet.
2043 |      // For simplicity, always leave P2SH+WITNESS+TAPROOT on except for the two
2044 |      // violating blocks.
2045 | -    uint32_t flags{SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_TAPROOT};
2046 | +    MandatoryFlags flags{MandatoryFlag<SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_TAPROOT>};
2047 |      const auto it{consensusparams.script_flag_exceptions.find(*Assert(block_index.phashBlock))};
2048 |      if (it != consensusparams.script_flag_exceptions.end()) {
2049 | -        flags = it->second;
2050 | +        flags &= it->second;
    ajtowns commented at 6:34 AM on August 25, 2023:

    This prevents script_flag_exceptions from adding rules; it can now only remove from the P2SH, SEGWIT, TAPROOT set.

  14. instagibbs commented at 12:42 PM on August 25, 2023: member

    concept ACK on first half, ~0 on latter commits

  15. ajtowns force-pushed on Aug 28, 2023
  16. ajtowns commented at 5:29 AM on August 28, 2023: contributor

    concept ACK on first half, ~0 on latter commits

    Split the first part out into #28354

  17. ajtowns renamed this:
    policy: Update -acceptnonstdtxn behaviour
    policy: Allow non-standard scripts with -acceptnonstdtxn=1
    on Aug 28, 2023
  18. DrahtBot removed the label CI failed on Aug 28, 2023
  19. ChrisMartl commented at 9:13 AM on August 29, 2023: none

    @ajtowns: what would be the behavior regarding non-standard scripts when -acceptnonstdtxn=0 ?

  20. ariard commented at 1:12 AM on August 30, 2023: contributor

    Mostly that STANDARD_SCRIPT_VERIFY_FLAGS violations sounds like something that acceptnonstdtxn should allow. But some particular examples:

    I think this is valuable for experimentation to allow non-standard scripts with -accetnonstdtxn=1, though I wonder if we wouldn’t wish more granularity e.g allow OP_SUCCESS / OP_NOP without bypassing IsStandardTx checks. I wonder if we might wish partial policy loosening deployment, e.g if a OP_NOP is used as a new data carrier though all the other checks on sanity of spent inputs / transaction size / etc are conserved. Related discussions in #27926.

  21. Retropex commented at 3:36 PM on August 30, 2023: none

    I'm not sure I understand what this brings to Bitcoin could you elaborate?

  22. moonsettler commented at 6:50 PM on August 30, 2023: none

    I'm not sure I understand what this brings to Bitcoin could you elaborate?

    it's a rationalization of node behavior. removes some logical inconsistencies related to standardness checks and relay policy and gives you better control over your own node.

    sometimes you remove weirdness instead of adding new functionality.

  23. instagibbs commented at 6:52 PM on August 30, 2023: member

    I don't know if it's particularly weird to "protect" upgrade hooks in testnets, but I also know that people can just go do their own thing and then mine a block with violations of these rules, hence my ~0

  24. in src/validation.cpp:2013 in 091d52be4b outdated 
    2008 | +// Helpers to ensure that GetBlockScriptFlags only returns a superset of MANDATORY_SCRIPT_VERIFY_FLAGS
2009 | +// (otherwise unminable txs could end up in the mempool)
2010 | +template<uint32_t FLAG>
2011 | +struct MandatoryFlagChecked
2012 | +{
2013 | +    static_assert((FLAG & MANDATORY_SCRIPT_VERIFY_FLAGS) == FLAG, "MandatoryFlag not included in MANDATORY_SCRIPT_VERIFY_FLAGS");
    ajtowns commented at 8:10 PM on September 1, 2023:

    Okay, I think there's a reasonable way of doing this with C++20 consteval:

    class MandatoryFlags {
private:
    uint32_t value{0};
    struct CheckFlag {
        uint32_t flag;
        consteval CheckFlag(uint32_t f) {
            if ((f & MANDATORY_SCRIPT_VERIFY_FLAGS) != f) {
                throw "Block script flag not included in MANDATORY_SCRIPT_VERIFY_FLAGS";
            }
            flag = f;
        }
    };
public:
    explicit MandatoryFlags(const CheckFlag& flags) { value |= flags.flag; }
    void operator|=(const CheckFlag& flags) { value |= flags.flag; }
    void operator&=(uint32_t flags) { value &= flags; }
    operator uint32_t() const { return value; }
};

    then declare MandatoryFlags flags; and you can just say flags |= SCRIPT_VERIFY_DERSIG; as normal, but if you give a non-mandatory flag, you get:

    validation.cpp:2072:14: error: call to consteval function '(anonymous namespace)::MandatoryFlags::CheckFlag::CheckFlag' is not a constant expression
    flags |= SCRIPT_VERIFY_MINIMALIF;
             ^
validation.cpp:2018:17: note: subexpression not valid in a constant expression
                throw "Block script flag not included in MANDATORY_SCRIPT_VERIFY_FLAGS";
                ^

    and saying flags |= var also gives you a decent error:

    validation.cpp:2049:18: error: call to consteval function '(anonymous namespace)::MandatoryFlags::CheckFlag::CheckFlag' is not a constant expression
        flags |= var;
                 ^
validation.cpp:2049:18: note: initializer of 'var' is not a constant expression
validation.cpp:2046:20: note: declared here
        const auto var = it->second;
                   ^
  25. ajtowns force-pushed on Sep 1, 2023
  26. ajtowns commented at 8:30 PM on September 1, 2023: contributor

    Updated to replace the template magic enforcing consistency between GetBlockScriptFlags and MANDATORY_SCRIPT_VERIFY_FLAGS with a comment.

  27. ajtowns commented at 8:43 PM on September 1, 2023: contributor

    I think this is valuable for experimentation to allow non-standard scripts with -accetnonstdtxn=1, though I wonder if we wouldn’t wish more granularity e.g allow OP_SUCCESS / OP_NOP without bypassing IsStandardTx checks. I wonder if we might wish partial policy loosening deployment, e.g if a OP_NOP is used as a new data carrier though all the other checks on sanity of spent inputs / transaction size / etc are conserved.

    I think any time you want more granularity, you should just define the new standardness rule and implement it. Examples:

    Using -acceptnonstdtxn is useful for experimenting before you get to that point (or while you're not confident that the code implementing it is really safe to run, but you want to see the txs that use those rules anyway).

  28. ajtowns marked this as ready for review on Sep 1, 2023
  29. DrahtBot added the label CI failed on Sep 3, 2023
  30. DrahtBot removed the label CI failed on Sep 5, 2023
  31. luke-jr commented at 7:29 PM on September 5, 2023: member

    I think we should distinguish at least between simply non-standard transactions, and ones that use opcodes reserved for future functionality. Maybe only bypass the latter for -acceptnonstdtxn=2 or something?

  32. moonsettler commented at 10:12 PM on September 5, 2023: none

    @luke-jr that thought crossed my mind too. so long the option is available granularity is not a bad thing at all.

  33. ajtowns commented at 12:55 AM on September 6, 2023: contributor

    I think we should distinguish at least between simply non-standard transactions, and ones that use opcodes reserved for future functionality. Maybe only bypass the latter for -acceptnonstdtxn=2 or something?

    I guess the difference would be:

    • malleability:
      • SCRIPT_VERIFY_MINIMALDATA
      • SCRIPT_VERIFY_CLEANSTACK
      • SCRIPT_VERIFY_MINIMALIF
      • SCRIPT_VERIFY_NULLFAIL
      • SCRIPT_VERIFY_LOW_S
    • upgrades:
      • SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_NOPS
      • SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_WITNESS_PROGRAM
      • SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_TAPROOT_VERSION
      • SCRIPT_VERIFY_DISCOURAGE_OP_SUCCESS
      • SCRIPT_VERIFY_DISCOURAGE_UPGRADABLE_PUBKEYTYPE
    • weirdness:
      • SCRIPT_VERIFY_CONST_SCRIPTCODE
      • SCRIPT_VERIFY_STRICTENC
      • SCRIPT_VERIFY_WITNESS_PUBKEYTYPE

    Not really seeing the point in differentiating between them though; all the examples I have are where you want to mess with the upgradable things.

  34. ariard commented at 12:30 AM on September 9, 2023: contributor

    I think any time you want more granularity, you should just define the new standardness rule and implement it.

    Yes this is one policy rules design viewpoint. The pitfall I can think of is in term of DoS protection of full-nodes. Let’s say in the future we discover a new DoS vector on new deployed segwit versions (e.g grafroot spends) and max transaction size. Max transaction size (MAX_STANDARD_TX_WEIGHT) is currently enforced IsStandardTx, lowering this limit would a plausible fix to this DoS vector, however it would come at the ecosystem cost of breaking all the data carriage use-case relying on current MAX_STANDARD_TX_WEIGHT. Those node operators if they have high-performance full-nodes would have an interest to keep those loose default, and we’ll start to have mempool partitioning in the ecosystem (sounds bad for decentralization), I think.

    All of that hypothetical, though it sounds the most-granular policy loosening allowing the use-case can be a better policy rules design thumb rule. I wonder if here a new acceptnonstdscripts=1 would fit ? If it’s for experimentation-only, the current code changes could be also signet / testnet /regtest only.

  35. DrahtBot added the label CI failed on Jan 12, 2024
  36. tests: in p2p_segwit, check non-mandatory errors with -acceptnonstdtxn=0 node
    Prepare for updating -acceptnonstdtxn to allow txns that violate
STANDARD_SCRIPT_VERIFY_FLAGS but not MANDATORY_SCRIPT_VERIFY_FLAGS by
checking the non-mandatory flags with node that enforces standardness.
    4a2a5e0d08
  37. validation: Check only MANDATORY_SCRIPT_VERIFY_FLAGS when -acceptnonstdtxn is set 2320ecebf3
  38. ajtowns force-pushed on Feb 13, 2024
  39. DrahtBot removed the label CI failed on Feb 13, 2024
  40. achow101 requested review from instagibbs on Apr 9, 2024
  41. instagibbs commented at 6:59 AM on April 10, 2024: member

    Leaning on -0 now for this PR.

    Unfortunately I think mining pools may start hacking things off with their modifications, including flipping standardness switch in mainnet. From what I can tell, Marathon was doing exactly this feedback was given to introduce back some limits, such as upgrade hooks.

  42. ajtowns commented at 8:37 AM on April 10, 2024: contributor

    Note that the concept aks in the drahtbot report were mostly/entirely for the "don't do this on testnet by default" aspect of this PR which was split out into #28354.

  43. maflcko commented at 8:49 AM on April 10, 2024: member

    Maybe reopen as a fresh pull request, if still relevant, so that there is a clean and easy to follow discussion?

  44. ajtowns closed this on Apr 10, 2024
  45. ajtowns commented at 9:16 AM on April 10, 2024: contributor

    Closed and re-opened fresh as #29843

  46. bitcoin locked this on Apr 10, 2025
Contributors
ajtownsDrahtBotdarosiormoonsettlersipainstagibbsChrisMartlariardRetropexluke-jrmaflcko


instagibbs

Labels
TX fees and policy
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: 2026-05-07 12:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me