The -acceptnonstdtxn footgun is only enabled for regtest/testnet due to several shortcomings:
This pull adds a new optional RPC parameter to ignore specific reject reasons for a specific tx.
48@@ -49,7 +49,7 @@ static void AssembleBlock(benchmark::Bench& bench)
49
50 for (const auto& txr : txs) {
51 TxValidationState state;
52- bool ret{::AcceptToMemoryPool(*test_setup.m_node.mempool, state, txr, nullptr /* plTxnReplaced */, false /* bypass_limits */)};
53+ bool ret{::AcceptToMemoryPool(*test_setup.m_node.mempool, state, txr, nullptr /* plTxnReplaced */, {} /* ignore_rejects */, false /* bypass_limits */)};
134 if (whichType == TxoutType::NULL_DATA)
135 nDataOut++;
136 else if ((whichType == TxoutType::MULTISIG) && (!permit_bare_multisig)) {
137- reason = "bare-multisig";
138- return false;
139+ MAYBE_REJECT(reason, "bare-multisig", ignore_rejects);
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, 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.
I think this needs more motivation. Without a strong reason to do this, I tend to agree with the comments on #7533:
sendrawtransaction), so I don’t see how this pull makes any difference. If you have specific concerns, it would be good to elaborate them with an example, so that it is easier to understand them.
To extend my reply a bit: Currently it is only possible to override the baked-in behavior for some very specific policy checks via command line (e.g. -dustrelayfee) for all network and rpc transactions. The goal of this change is to allow to override policy reject reasons while also giving the user more flexibility than an “on-off” -acceptnonstdtxn setting. The reason that -acceptnonstdtxn is disabled is that it wouldn’t be safe to turn on in normal operation for the default user. So I think the flexibility to have a per-tx override without restarting the node is useful. Maybe even outside the primary use case of one-off manual sends.
Simply having broader categories (or even a single category) doesn’t solve the problem that policy changes between versions.
Concept NACK on exposing this for mainnet. That seems like a huge footgun, even with the warning. What is the use case? We should never treat our own transactions differently than ones coming from the network.
I think it’s pretty ugly in any case, but can imagine it’s useful for testing.
invalidateblock may lead to issues when used without thinking.
Where is the footgun? Many policies are based on DoS risk, which doesn’t exist if there’s a user actively trying to add a transaction.
As for use cases, I know one real-world case where a user accidentally used an uncompressed key for a segwit address. He’s figured out his mistake, and in theory he should be able to recover his coins, but he hasn’t convinced any miners (last I checked) to modify their software to accept it. (This PR might not be enough to get that one past, but it demonstrates that there are legitimate needs to override policies.)
testmempoolaccept will currently tell you exactly which policy a tx is failing; if a user wants to know “does my transaction pass just consensus rules otherwise?” I think ability to toggle on/off might make sense. But if a user is trying to get a nonstandard tx included in a block and can’t get a miner to accept it directly, how does this help? Core nodes still wouldn’t (and shouldn’t) relay it. Unless someone is planning to mine the tx themselves (not the typical user?) it would just be garbage sitting in their local mempool right?
Correct, the transaction won’t relay typically. (Unless you know that you are running an old version of Bitcoin Core to relay a tx that future versions of Bitcoin Core consider policy-“valid”, but this is probably an edge case). Thus, I think the primary user of this feature is a miner. (And maybe some power users that know exactly what they are doing).
I think ability to toggle on/off might make sense.
I am happy to explore this direction. Though, testmempoolaccept will only tell you the first policy-failure reason, not the second one. So I was thinking that it might be safer to supply all failure reasons explicitly instead of hoping there was only one.
Another option would be for testmempoolaccept to continue and collect all policy-failures on the way and return all of them instead of only the first? This would make an on/off switch safe again, I believe.
But if a user is trying to get a nonstandard tx included in a block and can’t get a miner to accept it directly, how does this help?
This is how a miner would accept it directly.
Another option would be for testmempoolaccept to continue and collect all policy-failures on the way and return all of them instead of only the first?
This seems like a good idea regardless.
This is how a miner would accept it directly.
Ah ok. Then, wrt maintainability, it still seems simpler to just set policy on/off entirely?
Another option would be for
testmempoolacceptto continue and collect all policy-failures on the way and return all of them instead of only the first? This would make an on/off switch safe again, I believe.
That sounds nice. Although I’d say it’s tricky to implement - what we might consider individual policies aren’t really mutually exclusive and we usually stop after the first violation because later checks would break / say the same thing but be more expensive. For example too small or scriptSig size. Would we run EvalScript multiple times for the standard flags? I suppose this is where inputs specifying which policies to exclude would make sense, but still requires policy to be well-enumerated/defined.
Although I’d say it’s tricky to implement - what we might consider individual policies aren’t really mutually exclusive and we usually stop after the first violation because later checks would break / say the same thing but be more expensive.
Absolutely agree with this. Mempool acceptance logic should be optimized towards simplicity and performance. Making the internal logic more complex so that we can return additional debugging information over an interface that we can’t guarantee to be stable doesn’t seem like the right decision to me.
The policy rejection reasons should live in ./src/policy, just like the
other policy rejection reasons (e.g. IsStandardTx)
This is a pure refactor to pass ignore_rejects from the
sendrawtransaction and testmempoolaccept RPCs to policy.
Only default-constructed (empty) sets are passed and never used, so this
doesn't change behavior.
Although I’d say it’s tricky to implement - what we might consider individual policies aren’t really mutually exclusive and we usually stop after the first violation because later checks would break / say the same thing but be more expensive.
Absolutely agree with this. Mempool acceptance logic should be optimized towards simplicity and performance. Making the internal logic more complex so that we can return additional debugging information over an interface that we can’t guarantee to be stable doesn’t seem like the right decision to me.
We already return the failure reason, and it is already not stable, so this wouldn’t change anything fundamentally. Also, this doesn’t change performance of mempool acceptance logic if not enabled. Yes, it might run checks that aren’t mutually exclusive, but what is the problem with that if the user explicitly asks to do that. A user might also rescan the wallet in a loop, even though it has already been rescanned.
🐙 This pull request conflicts with the target branch and needs rebase.
Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.