policy: Allow non-standard scripts with -acceptnonstdtxn=1 #28334
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Concept ACK! |
|
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 |
|
Concept ACK |
Can you give a bit of motivation for this second part? |
b76d2d6 to
a656ecc
Compare
Mostly that
|
src/validation.cpp
Outdated
| @@ -2016,33 +2042,33 @@ static unsigned int GetBlockScriptFlags(const CBlockIndex& block_index, const Ch | |||
| // mainnet. | |||
| // For simplicity, always leave P2SH+WITNESS+TAPROOT on except for the two | |||
| // violating blocks. | |||
| uint32_t flags{SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_TAPROOT}; | |||
| MandatoryFlags flags{MandatoryFlag<SCRIPT_VERIFY_P2SH | SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_TAPROOT>}; | |||
There was a problem hiding this comment.
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>;
src/validation.cpp
Outdated
| const auto it{consensusparams.script_flag_exceptions.find(*Assert(block_index.phashBlock))}; | ||
| if (it != consensusparams.script_flag_exceptions.end()) { | ||
| flags = it->second; | ||
| flags &= it->second; |
There was a problem hiding this comment.
This prevents script_flag_exceptions from adding rules; it can now only remove from the P2SH, SEGWIT, TAPROOT set.
|
concept ACK on first half, |
a656ecc to
091d52b
Compare
Split the first part out into #28354 |
|
@ajtowns: what would be the behavior regarding non-standard scripts when -acceptnonstdtxn=0 ? |
I think this is valuable for experimentation to allow non-standard scripts with |
|
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. |
|
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 |
091d52b to
f09bb66
Compare
src/validation.cpp
Outdated
| template<uint32_t FLAG> | ||
| struct MandatoryFlagChecked | ||
| { | ||
| static_assert((FLAG & MANDATORY_SCRIPT_VERIFY_FLAGS) == FLAG, "MandatoryFlag not included in MANDATORY_SCRIPT_VERIFY_FLAGS"); |
There was a problem hiding this comment.
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;
^
I think any time you want more granularity, you should just define the new standardness rule and implement it. Examples:
Using |
|
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 |
|
@luke-jr that thought crossed my mind too. so long the option is available granularity is not a bad thing at all. |
I guess the difference would be:
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. |
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 ( 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 |
…n=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.
f09bb66 to
2320ece
Compare
|
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. |
|
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. |
|
Maybe reopen as a fresh pull request, if still relevant, so that there is a clean and easy to follow discussion? |
|
Closed and re-opened fresh as #29843 |
This PR changes
-acceptnonstdtxn=1so that it also skips thenon-mandatoryscript checks, allowing txs that (eg) use OP_NOPx or OP_SUCCESS into the mempool. This remains only available on test nets.