policy: Allow non-standard scripts with -acceptnonstdtxn=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.

DrahtBot commented at 4:30 am on August 24, 2023: 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 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.

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.

DrahtBot added the label TX fees and policy on Aug 24, 2023

darosior commented at 7:43 am on August 24, 2023: member

Concept ACK. Makes sense to me.

moonsettler commented at 11:08 am on August 24, 2023: none

Concept ACK!

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.

sipa commented at 12:41 pm on August 24, 2023: member

Concept ACK

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?

DrahtBot added the label CI failed on Aug 24, 2023

ajtowns force-pushed on Aug 25, 2023

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

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:

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

or

0validation.cpp:2013:5: error: static_assert failed due to requirement '(16384U & MANDATORY_SCRIPT_VERIFY_FLAGS) == 16384U' "MandatoryFlag not included in MANDATORY_SCRIPT_VERIFY_FLAGS"
1....
2validation.cpp:2071:14: note: in instantiation of variable template specialization '(anonymous namespace)::MandatoryFlag<16384>' requested here
3    flags |= MandatoryFlag<SCRIPT_VERIFY_NULLFAIL>;

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.

instagibbs commented at 12:42 pm on August 25, 2023: member

concept ACK on first half, ~0 on latter commits

ajtowns force-pushed on Aug 28, 2023

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

ajtowns renamed this:
~~policy: Update -acceptnonstdtxn behaviour~~
policy: Allow non-standard scripts with -acceptnonstdtxn=1
on Aug 28, 2023

DrahtBot removed the label CI failed on Aug 28, 2023

ChrisMartl commented at 9:13 am on August 29, 2023: none

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

ariard commented at 1:12 am on August 30, 2023: member

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.

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?

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.

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

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:

 0class MandatoryFlags {
 1private:
 2    uint32_t value{0};
 3    struct CheckFlag {
 4        uint32_t flag;
 5        consteval CheckFlag(uint32_t f) {
 6            if ((f & MANDATORY_SCRIPT_VERIFY_FLAGS) != f) {
 7                throw "Block script flag not included in MANDATORY_SCRIPT_VERIFY_FLAGS";
 8            }
 9            flag = f;
10        }
11    };
12public:
13    explicit MandatoryFlags(const CheckFlag& flags) { value |= flags.flag; }
14    void operator|=(const CheckFlag& flags) { value |= flags.flag; }
15    void operator&=(uint32_t flags) { value &= flags; }
16    operator uint32_t() const { return value; }
17};

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:

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

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

0validation.cpp:2049:18: error: call to consteval function '(anonymous namespace)::MandatoryFlags::CheckFlag::CheckFlag' is not a constant expression
1        flags |= var;
2                 ^
3validation.cpp:2049:18: note: initializer of 'var' is not a constant expression
4validation.cpp:2046:20: note: declared here
5        const auto var = it->second;
6                   ^

ajtowns force-pushed on Sep 1, 2023

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.

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

ajtowns marked this as ready for review on Sep 1, 2023

DrahtBot added the label CI failed on Sep 3, 2023

DrahtBot removed the label CI failed on Sep 5, 2023

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?

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.

ajtowns commented at 0: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.

ariard commented at 0:30 am on September 9, 2023: member

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.

DrahtBot added the label CI failed on Jan 12, 2024

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

validation: Check only MANDATORY_SCRIPT_VERIFY_FLAGS when -acceptnonstdtxn is set 2320ecebf3

ajtowns force-pushed on Feb 13, 2024

DrahtBot removed the label CI failed on Feb 13, 2024

achow101 requested review from instagibbs on Apr 9, 2024

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.

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.

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?

ajtowns closed this on Apr 10, 2024

ajtowns commented at 9:16 am on April 10, 2024: contributor

Closed and re-opened fresh as #29843

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

Code Coverage

Reviews

Conflicts