net, validation: don’t punish peers for consensus-invalid txs #33050

pull ajtowns wants to merge 3 commits into bitcoin:master from ajtowns:202507-nopunishtx changing 14 files +63 −140
  1. ajtowns commented at 10:38 am on July 24, 2025: contributor

    Because we do not discourage nodes for transactions we consider non-standard, we don’t get any DoS protection from this check in adversarial scenarios, so remove the check entirely both to simplify the code and reduce the risk of splitting the network due to changes in tx relay policy.

    Then, because we no longer make use of the distinction between consensus and standardness failures during script validation, don’t re-validate each script with only-consensus rules, reducing the cost to us of transactions that we won’t relay.

  2. DrahtBot commented at 10:38 am on July 24, 2025: contributor

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

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33050.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK darosior, sipa, glozow, achow101

    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:

    • #32575 (refactor: Split multithreaded case out of CheckInputScripts by fjahr)
    • #29843 (policy: Allow non-standard scripts with -acceptnonstdtxn=1 (test nets only) by ajtowns)

    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. ajtowns commented at 10:53 am on July 24, 2025: contributor

    See also #33012.

    DoS banning was introduced in #517. DoS banning for some txs was removed in #896, #2540 and #3843, and then extended back to cover various post-p2sh soft forks in #26291. DoS banning was changed to be less banny in #14929, changed to discouragement in #19219 and changed from score-based to instant in #29575.

    Also related is the RECENT_CONSENSUS_CHANGE validation state, which was removed in #31269, but which was intended to prevent banning/discouragement of peers that would relay txs/blocks that were invalid due to recent consensus rule changes in order to reduce chances of a network split.

  4. ajtowns force-pushed on Jul 24, 2025
  5. DrahtBot added the label CI failed on Jul 24, 2025
  6. ajtowns force-pushed on Jul 24, 2025
  7. in test/functional/p2p_invalid_tx.py:76 in 46f9f8069f outdated 
    72@@ -73,7 +73,7 @@ def run_test(self):
73             tx = template.get_tx()
74             node.p2ps[0].send_txs_and_test(
75                 [tx], node, success=False,
76-                expect_disconnect=template.expect_disconnect,
77+                expect_disconnect=False,
    darosior commented at 12:57 pm on July 24, 2025:
    You might as well drop expect_disconnect in the templates then, instead of setting it to False both there and here? Also the reconnect logic just below here is now dead code.
  8. in src/validation.cpp:2221 in a4aa23a9c5 outdated 
    2238-                    //
2239-                    // Avoid reporting a mandatory script check failure with a non-mandatory error
2240-                    // string by reporting the error from the second check.
2241-                    result = mandatory_result;
2242-                }
2243+                return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(result->first)), result->second);
    darosior commented at 1:02 pm on July 24, 2025:

    This could make a non-standard transaction error with “non-mandatory-script-verify-flag”, which would be confusing. I think we can just drop the mandatoriness language.

    0- return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(result->first)), result->second);
1+ return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("script validation failed (%s)", ScriptErrorString(result->first)), result->second);
    ajtowns commented at 4:21 am on July 25, 2025:
    Maybe “mempool-script-verify-flag-failed” and “block-script-verify-flag-failed” would be better? I didn’t want to change that here since it’s a bit more intrusive for no functional change. Could probably be a scripted-diff in a followup?
    darosior commented at 1:40 pm on July 25, 2025:

    If only the non-standard error message is renamed, and “block-script-verify-flag-failed” is left as a followup, this should not meaningfully affect the size of the diff at all. Here is a diff which does this with your “mempool-script-verify-flag-failed” suggestion:

      0diff --git a/src/validation.cpp b/src/validation.cpp
  1index 3acdc6f63a1..a73ebc56eeb 100644
  2--- a/src/validation.cpp
  3+++ b/src/validation.cpp
  4@@ -2218,7 +2218,7 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
  5             // arguments) or due to new consensus rules introduced in
  6             // soft forks.
  7             if (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) {
  8-                return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(result->first)), result->second);
  9+                return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("mempool-script-verify-flag-failed (%s)", ScriptErrorString(result->first)), result->second);
 10             } else {
 11                 return state.Invalid(TxValidationResult::TX_CONSENSUS, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(result->first)), result->second);
 12             }
 13diff --git a/test/functional/data/invalid_txs.py b/test/functional/data/invalid_txs.py
 14index f34c5375090..57080fb271c 100644
 15--- a/test/functional/data/invalid_txs.py
 16+++ b/test/functional/data/invalid_txs.py
 17@@ -223,7 +223,8 @@ class CreateSumTooLarge(BadTxTemplate):
 18 
 19 
 20 class InvalidOPIFConstruction(BadTxTemplate):
 21-    reject_reason = "non-mandatory-script-verify-flag (Invalid OP_IF construction)"
 22+    reject_reason = "mempool-script-verify-flag-failed (Invalid OP_IF construction)"
 23+    block_reject_reason = "mandatory-script-verify-flag-failed (Invalid OP_IF construction)"
 24     expect_disconnect = False
 25 
 26     def get_tx(self):
 27@@ -277,7 +278,7 @@ def getDisabledOpcodeTemplate(opcode):
 28 
 29 class NonStandardAndInvalid(BadTxTemplate):
 30     """A non-standard transaction which is also consensus-invalid should return the first error."""
 31-    reject_reason = "non-mandatory-script-verify-flag (Using OP_CODESEPARATOR in non-witness script)"
 32+    reject_reason = "mempool-script-verify-flag-failed (Using OP_CODESEPARATOR in non-witness script)"
 33     block_reject_reason = "mandatory-script-verify-flag-failed (OP_RETURN was encountered)"
 34     expect_disconnect = False
 35     valid_in_block = False
 36diff --git a/test/functional/feature_cltv.py b/test/functional/feature_cltv.py
 37index fe502515721..75cd38a98bf 100755
 38--- a/test/functional/feature_cltv.py
 39+++ b/test/functional/feature_cltv.py
 40@@ -154,7 +154,7 @@ class BIP65Test(BitcoinTestFramework):
 41             cltv_invalidate(spendtx, i)
 42 
 43             blk_rej = "mandatory-script-verify-flag-failed"
 44-            tx_rej = "non-mandatory-script-verify-flag"
 45+            tx_rej = "mempool-script-verify-flag-failed"
 46             expected_cltv_reject_reason = [
 47                 " (Operation not valid with the current stack size)",
 48                 " (Negative locktime)",
 49diff --git a/test/functional/feature_dersig.py b/test/functional/feature_dersig.py
 50index 0828fdb601e..eec1559a13e 100755
 51--- a/test/functional/feature_dersig.py
 52+++ b/test/functional/feature_dersig.py
 53@@ -121,8 +121,8 @@ class BIP66Test(BitcoinTestFramework):
 54                 'txid': spendtx_txid,
 55                 'wtxid': spendtx_wtxid,
 56                 'allowed': False,
 57-                'reject-reason': 'non-mandatory-script-verify-flag (Non-canonical DER signature)',
 58-                'reject-details': 'non-mandatory-script-verify-flag (Non-canonical DER signature), ' +
 59+                'reject-reason': 'mempool-script-verify-flag-failed (Non-canonical DER signature)',
 60+                'reject-details': 'mempool-script-verify-flag-failed (Non-canonical DER signature), ' +
 61                                   f"input 0 of {spendtx_txid} (wtxid {spendtx_wtxid}), spending {coin_txid}:0"
 62             }],
 63             self.nodes[0].testmempoolaccept(rawtxs=[spendtx.serialize().hex()], maxfeerate=0),
 64diff --git a/test/functional/feature_nulldummy.py b/test/functional/feature_nulldummy.py
 65index 0fca9791480..cd7c4491a27 100755
 66--- a/test/functional/feature_nulldummy.py
 67+++ b/test/functional/feature_nulldummy.py
 68@@ -37,7 +37,7 @@ from test_framework.util import (
 69 from test_framework.wallet import getnewdestination
 70 from test_framework.wallet_util import generate_keypair
 71 
 72-NULLDUMMY_TX_ERROR = "non-mandatory-script-verify-flag (Dummy CHECKMULTISIG argument must be zero)"
 73+NULLDUMMY_TX_ERROR = "mempool-script-verify-flag-failed (Dummy CHECKMULTISIG argument must be zero)"
 74 NULLDUMMY_BLK_ERROR = "mandatory-script-verify-flag-failed (Dummy CHECKMULTISIG argument must be zero)"
 75 
 76 def invalidate_nulldummy_tx(tx):
 77diff --git a/test/functional/feature_segwit.py b/test/functional/feature_segwit.py
 78index 3b864f8ceed..80266aa2e72 100755
 79--- a/test/functional/feature_segwit.py
 80+++ b/test/functional/feature_segwit.py
 81@@ -177,8 +177,8 @@ class SegWitTest(BitcoinTestFramework):
 82         assert_equal(self.nodes[2].getbalance(), 20 * Decimal("49.999"))
 83 
 84         self.log.info("Verify unsigned p2sh witness txs without a redeem script are invalid")
 85-        self.fail_accept(self.nodes[2], "non-mandatory-script-verify-flag (Operation not valid with the current stack size)", p2sh_ids[NODE_2][P2WPKH][1], sign=False)
 86-        self.fail_accept(self.nodes[2], "non-mandatory-script-verify-flag (Operation not valid with the current stack size)", p2sh_ids[NODE_2][P2WSH][1], sign=False)
 87+        self.fail_accept(self.nodes[2], "mempool-script-verify-flag-failed (Operation not valid with the current stack size)", p2sh_ids[NODE_2][P2WPKH][1], sign=False)
 88+        self.fail_accept(self.nodes[2], "mempool-script-verify-flag-failed (Operation not valid with the current stack size)", p2sh_ids[NODE_2][P2WSH][1], sign=False)
 89 
 90         self.generate(self.nodes[0], 1)  # block 164
 91 
 92@@ -197,13 +197,13 @@ class SegWitTest(BitcoinTestFramework):
 93 
 94         self.log.info("Verify default node can't accept txs with missing witness")
 95         # unsigned, no scriptsig
 96-        self.fail_accept(self.nodes[0], "non-mandatory-script-verify-flag (Witness program hash mismatch)", wit_ids[NODE_0][P2WPKH][0], sign=False)
 97-        self.fail_accept(self.nodes[0], "non-mandatory-script-verify-flag (Witness program was passed an empty witness)", wit_ids[NODE_0][P2WSH][0], sign=False)
 98-        self.fail_accept(self.nodes[0], "non-mandatory-script-verify-flag (Operation not valid with the current stack size)", p2sh_ids[NODE_0][P2WPKH][0], sign=False)
 99-        self.fail_accept(self.nodes[0], "non-mandatory-script-verify-flag (Operation not valid with the current stack size)", p2sh_ids[NODE_0][P2WSH][0], sign=False)
100+        self.fail_accept(self.nodes[0], "mempool-script-verify-flag-failed (Witness program hash mismatch)", wit_ids[NODE_0][P2WPKH][0], sign=False)
101+        self.fail_accept(self.nodes[0], "mempool-script-verify-flag-failed (Witness program was passed an empty witness)", wit_ids[NODE_0][P2WSH][0], sign=False)
102+        self.fail_accept(self.nodes[0], "mempool-script-verify-flag-failed (Operation not valid with the current stack size)", p2sh_ids[NODE_0][P2WPKH][0], sign=False)
103+        self.fail_accept(self.nodes[0], "mempool-script-verify-flag-failed (Operation not valid with the current stack size)", p2sh_ids[NODE_0][P2WSH][0], sign=False)
104         # unsigned with redeem script
105-        self.fail_accept(self.nodes[0], "non-mandatory-script-verify-flag (Witness program hash mismatch)", p2sh_ids[NODE_0][P2WPKH][0], sign=False, redeem_script=witness_script(False, self.pubkey[0]))
106-        self.fail_accept(self.nodes[0], "non-mandatory-script-verify-flag (Witness program was passed an empty witness)", p2sh_ids[NODE_0][P2WSH][0], sign=False, redeem_script=witness_script(True, self.pubkey[0]))
107+        self.fail_accept(self.nodes[0], "mempool-script-verify-flag-failed (Witness program hash mismatch)", p2sh_ids[NODE_0][P2WPKH][0], sign=False, redeem_script=witness_script(False, self.pubkey[0]))
108+        self.fail_accept(self.nodes[0], "mempool-script-verify-flag-failed (Witness program was passed an empty witness)", p2sh_ids[NODE_0][P2WSH][0], sign=False, redeem_script=witness_script(True, self.pubkey[0]))
109 
110         # Coinbase contains the witness commitment nonce, check that RPC shows us
111         coinbase_txid = self.nodes[2].getblock(blockhash)['tx'][0]
112@@ -214,10 +214,10 @@ class SegWitTest(BitcoinTestFramework):
113         assert_equal(witnesses[0], '00' * 32)
114 
115         self.log.info("Verify witness txs without witness data are invalid after the fork")
116-        self.fail_accept(self.nodes[2], 'non-mandatory-script-verify-flag (Witness program hash mismatch)', wit_ids[NODE_2][P2WPKH][2], sign=False)
117-        self.fail_accept(self.nodes[2], 'non-mandatory-script-verify-flag (Witness program was passed an empty witness)', wit_ids[NODE_2][P2WSH][2], sign=False)
118-        self.fail_accept(self.nodes[2], 'non-mandatory-script-verify-flag (Witness program hash mismatch)', p2sh_ids[NODE_2][P2WPKH][2], sign=False, redeem_script=witness_script(False, self.pubkey[2]))
119-        self.fail_accept(self.nodes[2], 'non-mandatory-script-verify-flag (Witness program was passed an empty witness)', p2sh_ids[NODE_2][P2WSH][2], sign=False, redeem_script=witness_script(True, self.pubkey[2]))
120+        self.fail_accept(self.nodes[2], 'mempool-script-verify-flag-failed (Witness program hash mismatch)', wit_ids[NODE_2][P2WPKH][2], sign=False)
121+        self.fail_accept(self.nodes[2], 'mempool-script-verify-flag-failed (Witness program was passed an empty witness)', wit_ids[NODE_2][P2WSH][2], sign=False)
122+        self.fail_accept(self.nodes[2], 'mempool-script-verify-flag-failed (Witness program hash mismatch)', p2sh_ids[NODE_2][P2WPKH][2], sign=False, redeem_script=witness_script(False, self.pubkey[2]))
123+        self.fail_accept(self.nodes[2], 'mempool-script-verify-flag-failed (Witness program was passed an empty witness)', p2sh_ids[NODE_2][P2WSH][2], sign=False, redeem_script=witness_script(True, self.pubkey[2]))
124 
125         self.log.info("Verify default node can now use witness txs")
126         self.success_mine(self.nodes[0], wit_ids[NODE_0][P2WPKH][0], True)
127diff --git a/test/functional/mempool_accept.py b/test/functional/mempool_accept.py
128index 81c6ea3e5d2..e981933e2e4 100755
129--- a/test/functional/mempool_accept.py
130+++ b/test/functional/mempool_accept.py
131@@ -473,7 +473,7 @@ class MempoolAcceptanceTest(BitcoinTestFramework):
132         nested_anchor_spend.vout.append(CTxOut(nested_anchor_tx.vout[0].nValue - int(fee*COIN), script_to_p2wsh_script(CScript([OP_TRUE]))))
133 
134         self.check_mempool_result(
135-            result_expected=[{'txid': nested_anchor_spend.txid_hex, 'allowed': False, 'reject-reason': 'non-mandatory-script-verify-flag (Witness version reserved for soft-fork upgrades)'}],
136+            result_expected=[{'txid': nested_anchor_spend.txid_hex, 'allowed': False, 'reject-reason': 'mempool-script-verify-flag-failed (Witness version reserved for soft-fork upgrades)'}],
137             rawtxs=[nested_anchor_spend.serialize().hex()],
138             maxfeerate=0,
139         )
140diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py
141index edb5048acab..4b17cd619de 100755
142--- a/test/functional/p2p_segwit.py
143+++ b/test/functional/p2p_segwit.py
144@@ -690,13 +690,13 @@ class SegWitTest(BitcoinTestFramework):
145         # segwit activation.  Note that older bitcoind's that are not
146         # segwit-aware would also reject this for failing CLEANSTACK.
147         with self.nodes[0].assert_debug_log(
148-                expected_msgs=[spend_tx.txid_hex, 'was not accepted: non-mandatory-script-verify-flag (Witness program was passed an empty witness)']):
149+                expected_msgs=[spend_tx.txid_hex, 'was not accepted: mempool-script-verify-flag-failed (Witness program was passed an empty witness)']):
150             test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False)
151 
152         # Try to put the witness script in the scriptSig, should also fail.
153         spend_tx.vin[0].scriptSig = CScript([p2wsh_pubkey, b'a'])
154         with self.nodes[0].assert_debug_log(
155-                expected_msgs=[spend_tx.txid_hex, 'was not accepted: non-mandatory-script-verify-flag (Script evaluated without error but finished with a false/empty top stack element)']):
156+                expected_msgs=[spend_tx.txid_hex, 'was not accepted: mempool-script-verify-flag-failed (Script evaluated without error but finished with a false/empty top stack element)']):
157             test_transaction_acceptance(self.nodes[0], self.test_node, spend_tx, with_witness=False, accepted=False)
158 
159         # Now put the witness script in the witness, should succeed after
160@@ -1434,7 +1434,7 @@ class SegWitTest(BitcoinTestFramework):
161         sign_input_segwitv0(tx2, 0, script, tx.vout[0].nValue, key)
162 
163         # Should fail policy test.
164-        test_transaction_acceptance(self.nodes[0], self.test_node, tx2, True, False, 'non-mandatory-script-verify-flag (Using non-compressed keys in segwit)')
165+        test_transaction_acceptance(self.nodes[0], self.test_node, tx2, True, False, 'mempool-script-verify-flag-failed (Using non-compressed keys in segwit)')
166         # But passes consensus.
167         block = self.build_next_block()
168         self.update_witness_block_with_transactions(block, [tx2])
169@@ -1453,7 +1453,7 @@ class SegWitTest(BitcoinTestFramework):
170         sign_p2pk_witness_input(witness_script, tx3, 0, SIGHASH_ALL, tx2.vout[0].nValue, key)
171 
172         # Should fail policy test.
173-        test_transaction_acceptance(self.nodes[0], self.test_node, tx3, True, False, 'non-mandatory-script-verify-flag (Using non-compressed keys in segwit)')
174+        test_transaction_acceptance(self.nodes[0], self.test_node, tx3, True, False, 'mempool-script-verify-flag-failed (Using non-compressed keys in segwit)')
175         # But passes consensus.
176         block = self.build_next_block()
177         self.update_witness_block_with_transactions(block, [tx3])
178@@ -1470,7 +1470,7 @@ class SegWitTest(BitcoinTestFramework):
179         sign_p2pk_witness_input(witness_script, tx4, 0, SIGHASH_ALL, tx3.vout[0].nValue, key)
180 
181         # Should fail policy test.
182-        test_transaction_acceptance(self.nodes[0], self.test_node, tx4, True, False, 'non-mandatory-script-verify-flag (Using non-compressed keys in segwit)')
183+        test_transaction_acceptance(self.nodes[0], self.test_node, tx4, True, False, 'mempool-script-verify-flag-failed (Using non-compressed keys in segwit)')
184         block = self.build_next_block()
185         self.update_witness_block_with_transactions(block, [tx4])
186         test_witness_block(self.nodes[0], self.test_node, block, accepted=True)
187diff --git a/test/functional/rpc_packages.py b/test/functional/rpc_packages.py
188index 4fdc1785cdc..ae14baf953a 100755
189--- a/test/functional/rpc_packages.py
190+++ b/test/functional/rpc_packages.py
191@@ -122,8 +122,8 @@ class RPCPackagesTest(BitcoinTestFramework):
192         assert_equal(testres_bad_sig, self.independent_txns_testres + [{
193             "txid": tx_bad_sig_txid,
194             "wtxid": tx_bad_sig_wtxid, "allowed": False,
195-            "reject-reason": "non-mandatory-script-verify-flag (Operation not valid with the current stack size)",
196-            "reject-details": "non-mandatory-script-verify-flag (Operation not valid with the current stack size), " +
197+            "reject-reason": "mempool-script-verify-flag-failed (Operation not valid with the current stack size)",
198+            "reject-details": "mempool-script-verify-flag-failed (Operation not valid with the current stack size), " +
199                               f"input 0 of {tx_bad_sig_txid} (wtxid {tx_bad_sig_wtxid}), spending {coin['txid']}:{coin['vout']}"
200         }])

    Squashing this diff into a4aa23a9c5589fc922f0e708533400a8c410701f makes it go from (+51, -62) to (+56, -66) and avoids an arguably regression from #26291 ’s error messages fix. Although in any case i agree this is secondary and i’m happy to PR this diff in a followup if you feel strongly.

  9. darosior commented at 1:03 pm on July 24, 2025: member
    Concept ACK.
  10. glozow added the label P2P on Jul 24, 2025
  11. fanquake commented at 4:38 pm on July 24, 2025: member
    CI failure here is (#33015).
  12. mzumsande commented at 6:11 pm on July 24, 2025: contributor

    Because we do not discourage nodes for transactions we consider non-standard, we don’t get any DoS protection from this check in adversarial scenarios.

    Could you expand on the logic of this - is it just because the most effective DoS vectors known today happen to involve non-standard but valid transactions, or does this hold with more generality? What if the current DoS vectors involving non-standard txs are fixed in the future, and the most-effective known DoS vectors then will involve consensus-invalid txs - wouldn’t we want to go back to punishing peers then?

  13. ajtowns commented at 4:18 am on July 25, 2025: contributor

    Could you expand on the logic of this - is it just because the most effective DoS vectors known today happen to involve non-standard but valid transactions, or does this hold with more generality? What if the current DoS vectors involving non-standard txs are fixed in the future, and the most-effective known DoS vectors then will involve consensus-invalid txs - wouldn’t we want to go back to punishing peers then?

    A DoS attack needs two features: each instance needs to be relatively high cost, and you need to be able to cheaply create many instances. The “high cost” part comes from the consensus-valid and standard-compliant parts of the tx – for example a valid multisig script that checks 20 signatures, or a script that calculates many hashes. The non-standard/consensus-invalid parts is what gives you the “cheap repeatability” part – if the tx were standard and valid, you’d accept it into the mempool, and quickly reject other txs because they were double spends, or require them to bump the fee, eventually making the tx costly.

    Making the tx non-standard gives you an easy way to repeat the attack: because we reject the tx, we’ll have to re-evaluate a new tx that spends the same inputs with the same fee to see if it’s possibly valid. If all the txs are consensus-invalid the attacker won’t even risk paying fees to a miner, but even if they are, they can still send a million variations on a single tx to you, and only have to pay a single fee to a miner for whichever one is eventually confirmed.

    If we were in a world where there weren’t many standardness rules and consensus rules dominated mempool acceptance, then we probably wouldn’t want to discourage/ban for some consensus failures in that case – recent soft forks would make previously valid txs invalid for both block inclusion and mempool acceptance, so discouraging peers who sent us those sorts of transactions would increase the p2p network’s fragility, possibly leading to network splits. If we did want to selectively discourage peers who should have upgraded, that would suggest reinstating the RECENT_CONSENSUS_CHANGE logic linked to above. In that world, an attacker could DoS us with txs that violated recent consensus rules, without being banned/discouraged.

    The main DoS protection here is to limit how much tx validation can interfere with other processing, so not evaluating potentially expensive scripts a second time when we already know they’re not interesting is more of a step forwards.

  14. DrahtBot removed the label CI failed on Jul 25, 2025
  15. gmaxwell commented at 9:32 pm on July 30, 2025: contributor

    I don’t see why all standardness rules can’t be validated prior to doing any costly hashing or signature validation (sadly, they do require looking up inputs which is costly).

    If indeed standardness validation happens before costly operations then the fact that they drop txn without disconnecting the peer doesn’t really tell us anything about what should be done after performing expensive operations.

    But most critically the primary reason to disconnect (and potentially keep disconnected) a peer that submits a consensus invalid transaction is to prevent our finite connection slots from being consumed by peers which have adopted consensus rules which are incompatible with our own (and to prevent us from consuming theirs, too).

  16. fanquake referenced this in commit 4f27e8ca4d on Aug 1, 2025
  17. darosior commented at 8:14 pm on August 4, 2025: member

    I don’t see why all standardness rules can’t be validated prior to doing any costly hashing or signature validation

    I think it’s non-trivial but should be possible in Tapscript to validate all standardness rules before executing the leaf script. It would effectively be a tightening of standardness (runtime upgrade hook checks like the key version would be statically inspected instead), but not one that should cause any issue to non-pathological transactions. For non-Tapscript scripts, short of restricting standardness to Miniscript-compatible scripts, i am not sure it’s realistic. For instance, i am not sure we could come up with a generalist way to inspect whether a redeem script / witness script is going to fail the MINIMALIF check without executing it.

    But more importantly i am not sure it’s worth it. As far as i can tell the existing behaviour does not exist for DoS resistance as it’s trivial for an attacker to circumvent (although it seems to be the reason why it was originally introduced), but as an opportunistic way to boot off non-malicious misbehaving peers.

    But most critically the primary reason to disconnect […] is to prevent our finite connection slots from being consumed by peers which have adopted consensus rules which are incompatible

    And we do so very inconsistently. Any consensus-invalid transaction which has a non-standard header or a non-standard input before the consensus-invalid input will be treated as non-standard and the peer not disconnected. Plus as @ajtowns points out in #33012 (comment) we would already disconnect those peers after two incompatible blocks.

    It seems to me the current behaviour is the result of starting off with always disconnecting for script failure with #517 and progressively carving out exceptions (see #33050 (comment)). As a result we don’t consistently achieve the primary reason you state for this behaviour, but we still double our CPU DoS exposure.

    I think the tradeoff of getting rid of the opportunistic early-disconnection of misbehaving peers in order to halve our CPU DoS exposure is worth it.

  18. sipa commented at 5:07 pm on August 6, 2025: member

    Approach ACK.

    I agree that the current ability to distinguish consensus vs standardness failures doesn’t really achieve much. It is not the case that consensus-invalidity is inherently more harmful to us (in terms of DoS potential) than non-standardness violations, and attackers can today already mask any consensus-invalidity by causing a non-standardness to be detected first, evading the punishment.

    So it seems the benefit of this distinction is largely so we can kick peers with diverging consensus rules. That is worthwhile, but we have other measures that take care of this already: disconnecting ones that give invalid blocks, and actively seeking out more connections when we don’t learn about a new block for a while.

    It would be possible to improve upon this somewhat without double/triple execution in theory, if we could do a single execution that reported using something like #33012, but that’s a much more invasive change. If we don’t limit what code changes we’re willing to make, I think an ideal situation is something where execution runs once, with all flags enabled, and then returns:

    • If a “DoSy condition” (whether consensus or standardness) is reached, immediately (with what type of failure).
    • Otherwise, run until consensus failure or consensus success, and then return success/success+nonstandard/consensus failure.

    @gmaxwell

    I don’t see why all standardness rules can’t be validated prior to doing any costly hashing or signature validation (sadly, they do require looking up inputs which is costly).

    In Taproot I think that’s possible for signature validation at least, because execution paths can be determined without executing sigchecks. For other scripts, or other expensive operations in tapscript (like big OP_ROLLs) it seems much harder.

    But moreover, I think what we want here, for the goal of keeping our slots reserved for consensus-compatible clients, is determining whether a transaction is consensus-invalid even when it is non-standard. Absent that, it’s independently useful to avoid doing expensive checks if can determine rejection without that, but I think that can be done separately, and also, this goal is somewhat in conflict with determining consensus invalidity.

    I think the question really is: how much computational work are we willing to do, for an already-known-non-standard transaction, to determine it’s also consensus invalid do we can free the connection slot? If the answer is nothing, then this PR is the complete solution. If it’s more than that, then more complicated solutions may be considered.

  19. achow101 added this to the milestone 30.0 on Aug 7, 2025
  20. sipa commented at 1:07 pm on August 8, 2025: member
    @ajtowns Do you plan to address the feedback here?
  21. ajtowns force-pushed on Aug 8, 2025
  22. sipa commented at 3:02 pm on August 8, 2025: member
    ACK f3bf5e25d70710c35556df7c5659446ebd23efc5
  23. DrahtBot requested review from darosior on Aug 8, 2025
  24. darosior approved
  25. darosior commented at 5:24 pm on August 8, 2025: member
    ACK f3bf5e25d70710c35556df7c5659446ebd23efc5
  26. darosior referenced this in commit dda32199e3 on Aug 8, 2025
  27. ajtowns commented at 6:43 pm on August 8, 2025: contributor
    silent merge conflict with 33105 presumably?
  28. net_processing: drop MaybePunishNodeForTx 
    Do not discourage nodes even when they send us consensus invalid
transactions.

Because we do not discourage nodes for transactions we consider
non-standard, we don't get any DoS protection from this check in
adversarial scenarios, so remove the check entirely both to simplify the
code and reduce the risk of splitting the network due to changes in tx
relay policy.
    266dd0e10d
  29. darosior commented at 6:49 pm on August 8, 2025: member
    Unsure. Due to what?
  30. glozow commented at 6:49 pm on August 8, 2025: member

    silent merge conflict with 33105 presumably?

    Doesn’t seem like it edit: yes

  31. ajtowns commented at 6:49 pm on August 8, 2025: contributor
    “mandatory-script-verify” tx rejection in p2p_segwit.py
  32. validation: only check input scripts once 
    Previously, we would check failing input scripts twice when considering
a transaction for the mempool, in order to distinguish policy failures
from consensus failures. This allowed us both to provide a different
error message and to discourage peers for consensus failures. Because we
are no longer discouraging peers for consensus failures during tx relay,
and because checking a script can be expensive, only do this once.

Also renames non-mandatory-script-verify-flag error to
mempool-script-verify-flag-failed.
    b29ae9efdf
  33. tests: drop expect_disconnect behaviour for tx relay 876dbdfb47
  34. ajtowns force-pushed on Aug 8, 2025
  35. ajtowns commented at 8:30 pm on August 8, 2025: contributor
    rebased
  36. darosior approved
  37. darosior commented at 8:42 pm on August 8, 2025: member
    re-ACK 876dbdfb4702410dfd4037614dc9298a0c09c63e
  38. DrahtBot requested review from sipa on Aug 8, 2025
  39. sipa commented at 9:14 pm on August 8, 2025: member
    re-ACK 876dbdfb4702410dfd4037614dc9298a0c09c63e
  40. glozow commented at 8:11 pm on August 12, 2025: member
    ACK 876dbdfb4702410dfd4037614dc9298a0c09c63e
  41. achow101 commented at 9:28 pm on August 12, 2025: member
    ACK 876dbdfb4702410dfd4037614dc9298a0c09c63e
  42. achow101 merged this on Aug 12, 2025
  43. achow101 closed this on Aug 12, 2025


ajtowns DrahtBot darosior fanquake mzumsande gmaxwell sipa glozow achow101

Labels
P2P

Milestone
30.0

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: 2025-08-15 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me