Merge bitcoin/bitcoin#33050: net, validation: don't punish peers for consensus-invalid txs

876dbdf tests: drop expect_disconnect behaviour for tx relay (Anthony Towns)
b29ae9e validation: only check input scripts once (Anthony Towns)
266dd0e net_processing: drop MaybePunishNodeForTx (Anthony Towns)

Pull request description:

  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.

ACKs for top commit:
  achow101:
    ACK 876dbdf
  darosior:
    re-ACK 876dbdf
  sipa:
    re-ACK 876dbdf
  glozow:
    ACK 876dbdf

Tree-SHA512: 8bb0395766dde54fc48f7077b80b88e35581aa6e3054d6d65735965147abefffa7348f0850bb3d46f6c2541fd384ecd40a00a57fa653adabff8a35582e2d1811

Author: achow101

src/net_processing.cpp

@@ -583,12 +583,6 @@ class PeerManagerImpl final : public PeerManager
                                  bool via_compact_block, const std::string& message = "")
         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
 
-    /**
-     * Potentially disconnect and discourage a node based on the contents of a TxValidationState object
-     */
-    void MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state)
-        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex);
-
     /** Maybe disconnect a peer and discourage future connections from its address.
      *
      * @param[in]   pnode     The node to check.
@@ -1836,32 +1830,6 @@ void PeerManagerImpl::MaybePunishNodeForBlock(NodeId nodeid, const BlockValidati
     }
 }
 
-void PeerManagerImpl::MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state)
-{
-    PeerRef peer{GetPeerRef(nodeid)};
-    switch (state.GetResult()) {
-    case TxValidationResult::TX_RESULT_UNSET:
-        break;
-    // The node is providing invalid data:
-    case TxValidationResult::TX_CONSENSUS:
-        if (peer) Misbehaving(*peer, "");
-        return;
-    // Conflicting (but not necessarily invalid) data or different policy:
-    case TxValidationResult::TX_INPUTS_NOT_STANDARD:
-    case TxValidationResult::TX_NOT_STANDARD:
-    case TxValidationResult::TX_MISSING_INPUTS:
-    case TxValidationResult::TX_PREMATURE_SPEND:
-    case TxValidationResult::TX_WITNESS_MUTATED:
-    case TxValidationResult::TX_WITNESS_STRIPPED:
-    case TxValidationResult::TX_CONFLICT:
-    case TxValidationResult::TX_MEMPOOL_POLICY:
-    case TxValidationResult::TX_NO_MEMPOOL:
-    case TxValidationResult::TX_RECONSIDERABLE:
-    case TxValidationResult::TX_UNKNOWN:
-        break;
-    }
-}
-
 bool PeerManagerImpl::BlockRequestAllowed(const CBlockIndex* pindex)
 {
     AssertLockHeld(cs_main);
@@ -3034,8 +3002,6 @@ std::optional<node::PackageToValidate> PeerManagerImpl::ProcessInvalidTx(NodeId
         if (peer) AddKnownTx(*peer, parent_txid);
     }
 
-    MaybePunishNodeForTx(nodeid, state);
-
     return package_to_validate;
 }
 

src/validation.cpp

@@ -2191,34 +2191,17 @@ bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
         if (pvChecks) {
             pvChecks->emplace_back(std::move(check));
         } else if (auto result = check(); result.has_value()) {
+            // Tx failures never trigger disconnections/bans.
+            // This is so that network splits aren't triggered
+            // either due to non-consensus relay policies (such as
+            // non-standard DER encodings or non-null dummy
+            // arguments) or due to new consensus rules introduced in
+            // soft forks.
             if (flags & STANDARD_NOT_MANDATORY_VERIFY_FLAGS) {
-                // Check whether the failure was caused by a
-                // non-mandatory script verification check, such as
-                // non-standard DER encodings or non-null dummy
-                // arguments; if so, ensure we return NOT_STANDARD
-                // instead of CONSENSUS to avoid downstream users
-                // splitting the network between upgraded and
-                // non-upgraded nodes by banning CONSENSUS-failing
-                // data providers.
-                CScriptCheck check2(txdata.m_spent_outputs[i], tx, validation_cache.m_signature_cache, i,
-                        flags & ~STANDARD_NOT_MANDATORY_VERIFY_FLAGS, cacheSigStore, &txdata);
-                auto mandatory_result = check2();
-                if (!mandatory_result.has_value()) {
-                    return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("non-mandatory-script-verify-flag (%s)", ScriptErrorString(result->first)), result->second);
-                } else {
-                    // If the second check failed, it failed due to a mandatory script verification
-                    // flag, but the first check might have failed on a non-mandatory script
-                    // verification flag.
-                    //
-                    // Avoid reporting a mandatory script check failure with a non-mandatory error
-                    // string by reporting the error from the second check.
-                    result = mandatory_result;
-                }
+                return state.Invalid(TxValidationResult::TX_NOT_STANDARD, strprintf("mempool-script-verify-flag-failed (%s)", ScriptErrorString(result->first)), result->second);
+            } else {
+                return state.Invalid(TxValidationResult::TX_CONSENSUS, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(result->first)), result->second);
             }
-
-            // MANDATORY flag failures correspond to
-            // TxValidationResult::TX_CONSENSUS.
-            return state.Invalid(TxValidationResult::TX_CONSENSUS, strprintf("mandatory-script-verify-flag-failed (%s)", ScriptErrorString(result->first)), result->second);
         }
     }
 

test/functional/data/invalid_txs.py

@@ -73,9 +73,6 @@ class BadTxTemplate:
     # Only specified if it differs from mempool acceptance error.
     block_reject_reason = ""
 
-    # Do we expect to be disconnected after submitting this tx?
-    expect_disconnect = False
-
     # Is this tx considered valid when included in a block, but not for acceptance into
     # the mempool (i.e. does it violate policy but not consensus)?
     valid_in_block = False
@@ -93,7 +90,6 @@ def get_tx(self, *args, **kwargs):
 
 class OutputMissing(BadTxTemplate):
     reject_reason = "bad-txns-vout-empty"
-    expect_disconnect = True
 
     def get_tx(self):
         tx = CTransaction()
@@ -103,7 +99,6 @@ def get_tx(self):
 
 class InputMissing(BadTxTemplate):
     reject_reason = "bad-txns-vin-empty"
-    expect_disconnect = True
 
     # We use a blank transaction here to make sure
     # it is interpreted as a non-witness transaction.
@@ -119,7 +114,6 @@ def get_tx(self):
 # tree depth commitment (CVE-2017-12842)
 class SizeTooSmall(BadTxTemplate):
     reject_reason = "tx-size-small"
-    expect_disconnect = False
     valid_in_block = True
 
     def get_tx(self):
@@ -137,7 +131,6 @@ class BadInputOutpointIndex(BadTxTemplate):
     reject_reason = None
     # But fails in block
     block_reject_reason = "bad-txns-inputs-missingorspent"
-    expect_disconnect = False
 
     def get_tx(self):
         num_indices = len(self.spend_tx.vin)
@@ -151,7 +144,6 @@ def get_tx(self):
 
 class DuplicateInput(BadTxTemplate):
     reject_reason = 'bad-txns-inputs-duplicate'
-    expect_disconnect = True
 
     def get_tx(self):
         tx = CTransaction()
@@ -163,7 +155,6 @@ def get_tx(self):
 
 class PrevoutNullInput(BadTxTemplate):
     reject_reason = 'bad-txns-prevout-null'
-    expect_disconnect = True
 
     def get_tx(self):
         tx = CTransaction()
@@ -175,7 +166,6 @@ def get_tx(self):
 
 class NonexistentInput(BadTxTemplate):
     reject_reason = None  # Added as an orphan tx.
-    expect_disconnect = False
     # But fails in block
     block_reject_reason = "bad-txns-inputs-missingorspent"
 
@@ -189,7 +179,6 @@ def get_tx(self):
 
 class SpendTooMuch(BadTxTemplate):
     reject_reason = 'bad-txns-in-belowout'
-    expect_disconnect = True
 
     def get_tx(self):
         return create_tx_with_script(
@@ -198,23 +187,20 @@ def get_tx(self):
 
 class CreateNegative(BadTxTemplate):
     reject_reason = 'bad-txns-vout-negative'
-    expect_disconnect = True
 
     def get_tx(self):
         return create_tx_with_script(self.spend_tx, 0, amount=-1)
 
 
 class CreateTooLarge(BadTxTemplate):
     reject_reason = 'bad-txns-vout-toolarge'
-    expect_disconnect = True
 
     def get_tx(self):
         return create_tx_with_script(self.spend_tx, 0, amount=MAX_MONEY + 1)
 
 
 class CreateSumTooLarge(BadTxTemplate):
     reject_reason = 'bad-txns-txouttotal-toolarge'
-    expect_disconnect = True
 
     def get_tx(self):
         tx = create_tx_with_script(self.spend_tx, 0, amount=MAX_MONEY)
@@ -223,8 +209,7 @@ def get_tx(self):
 
 
 class InvalidOPIFConstruction(BadTxTemplate):
-    reject_reason = "mandatory-script-verify-flag-failed (Invalid OP_IF construction)"
-    expect_disconnect = True
+    reject_reason = "mempool-script-verify-flag-failed (Invalid OP_IF construction)"
 
     def get_tx(self):
         return create_tx_with_script(
@@ -235,7 +220,6 @@ def get_tx(self):
 class TooManySigopsPerBlock(BadTxTemplate):
     reject_reason = "bad-txns-too-many-sigops"
     block_reject_reason = "bad-blk-sigops, out-of-bounds SigOpCount"
-    expect_disconnect = False
 
     def get_tx(self):
         lotsa_checksigs = CScript([OP_CHECKSIG] * (MAX_BLOCK_SIGOPS))
@@ -247,7 +231,6 @@ def get_tx(self):
 
 class TooManySigopsPerTransaction(BadTxTemplate):
     reject_reason = "bad-txns-too-many-sigops"
-    expect_disconnect = False
     valid_in_block = True
 
     def get_tx(self):
@@ -270,15 +253,14 @@ def get_tx(self):
 
     return type('DisabledOpcode_' + str(opcode), (BadTxTemplate,), {
         'reject_reason': "disabled opcode",
-        'expect_disconnect': True,
         'get_tx': get_tx,
         'valid_in_block' : False
         })
 
 class NonStandardAndInvalid(BadTxTemplate):
-    """A non-standard transaction which is also consensus-invalid should return the consensus error."""
-    reject_reason = "mandatory-script-verify-flag-failed (OP_RETURN was encountered)"
-    expect_disconnect = True
+    """A non-standard transaction which is also consensus-invalid should return the first error."""
+    reject_reason = "mempool-script-verify-flag-failed (Using OP_CODESEPARATOR in non-witness script)"
+    block_reject_reason = "mandatory-script-verify-flag-failed (OP_RETURN was encountered)"
     valid_in_block = False
 
     def get_tx(self):

test/functional/feature_block.py

@@ -177,11 +177,6 @@ def run_test(self):
         for TxTemplate in invalid_txs.iter_all_templates():
             template = TxTemplate(spend_tx=attempt_spend_tx)
 
-            # belt-and-suspenders checking we won't pass up validating something
-            # we expect a disconnect from
-            if template.expect_disconnect:
-                assert not template.valid_in_block
-
             if template.valid_in_block:
                 continue
 
@@ -194,9 +189,12 @@ def run_test(self):
             if TxTemplate != invalid_txs.InputMissing:
                 self.sign_tx(badtx, attempt_spend_tx)
             badblock = self.update_block(blockname, [badtx])
+            reject_reason = (template.block_reject_reason or template.reject_reason)
+            if reject_reason.startswith("mempool-script-verify-flag-failed"):
+                reject_reason = "mandatory-script-verify-flag-failed" + reject_reason[33:]
             self.send_blocks(
                 [badblock], success=False,
-                reject_reason=(template.block_reject_reason or template.reject_reason),
+                reject_reason=reject_reason,
                 reconnect=True, timeout=2)
 
             self.move_tip(2)

test/functional/feature_cltv.py

@@ -153,12 +153,14 @@ def run_test(self):
             coin_vout = coin.prevout.n
             cltv_invalidate(spendtx, i)
 
+            blk_rej = "mandatory-script-verify-flag-failed"
+            tx_rej = "mempool-script-verify-flag-failed"
             expected_cltv_reject_reason = [
-                "mandatory-script-verify-flag-failed (Operation not valid with the current stack size)",
-                "mandatory-script-verify-flag-failed (Negative locktime)",
-                "mandatory-script-verify-flag-failed (Locktime requirement not satisfied)",
-                "mandatory-script-verify-flag-failed (Locktime requirement not satisfied)",
-                "mandatory-script-verify-flag-failed (Locktime requirement not satisfied)",
+                " (Operation not valid with the current stack size)",
+                " (Negative locktime)",
+                " (Locktime requirement not satisfied)",
+                " (Locktime requirement not satisfied)",
+                " (Locktime requirement not satisfied)",
             ][i]
             # First we show that this tx is valid except for CLTV by getting it
             # rejected from the mempool for exactly that reason.
@@ -169,8 +171,8 @@ def run_test(self):
                     'txid': spendtx_txid,
                     'wtxid': spendtx_wtxid,
                     'allowed': False,
-                    'reject-reason': expected_cltv_reject_reason,
-                    'reject-details': expected_cltv_reject_reason + f", input 0 of {spendtx_txid} (wtxid {spendtx_wtxid}), spending {coin_txid}:{coin_vout}"
+                    'reject-reason': tx_rej + expected_cltv_reject_reason,
+                    'reject-details': tx_rej + expected_cltv_reject_reason + f", input 0 of {spendtx_txid} (wtxid {spendtx_wtxid}), spending {coin_txid}:{coin_vout}"
                 }],
                 self.nodes[0].testmempoolaccept(rawtxs=[spendtx.serialize().hex()], maxfeerate=0),
             )
@@ -180,7 +182,7 @@ def run_test(self):
             block.hashMerkleRoot = block.calc_merkle_root()
             block.solve()
 
-            with self.nodes[0].assert_debug_log(expected_msgs=[f'Block validation error: {expected_cltv_reject_reason}']):
+            with self.nodes[0].assert_debug_log(expected_msgs=[f'Block validation error: {blk_rej + expected_cltv_reject_reason}']):
                 peer.send_and_ping(msg_block(block))
                 assert_equal(int(self.nodes[0].getbestblockhash(), 16), tip)
                 peer.sync_with_ping()

test/functional/feature_dersig.py

@@ -121,8 +121,8 @@ def run_test(self):
                 'txid': spendtx_txid,
                 'wtxid': spendtx_wtxid,
                 'allowed': False,
-                'reject-reason': 'mandatory-script-verify-flag-failed (Non-canonical DER signature)',
-                'reject-details': 'mandatory-script-verify-flag-failed (Non-canonical DER signature), ' +
+                'reject-reason': 'mempool-script-verify-flag-failed (Non-canonical DER signature)',
+                'reject-details': 'mempool-script-verify-flag-failed (Non-canonical DER signature), ' +
                                   f"input 0 of {spendtx_txid} (wtxid {spendtx_wtxid}), spending {coin_txid}:0"
             }],
             self.nodes[0].testmempoolaccept(rawtxs=[spendtx.serialize().hex()], maxfeerate=0),

test/functional/feature_nulldummy.py

@@ -37,8 +37,8 @@
 from test_framework.wallet import getnewdestination
 from test_framework.wallet_util import generate_keypair
 
-NULLDUMMY_ERROR = "mandatory-script-verify-flag-failed (Dummy CHECKMULTISIG argument must be zero)"
-
+NULLDUMMY_TX_ERROR = "mempool-script-verify-flag-failed (Dummy CHECKMULTISIG argument must be zero)"
+NULLDUMMY_BLK_ERROR = "mandatory-script-verify-flag-failed (Dummy CHECKMULTISIG argument must be zero)"
 
 def invalidate_nulldummy_tx(tx):
     """Transform a NULLDUMMY compliant tx (i.e. scriptSig starts with OP_0)
@@ -104,7 +104,7 @@ def run_test(self):
                                           addr=self.ms_address, amount=47,
                                           privkey=self.privkey)
         invalidate_nulldummy_tx(test2tx)
-        assert_raises_rpc_error(-26, NULLDUMMY_ERROR, self.nodes[0].sendrawtransaction, test2tx.serialize_with_witness().hex(), 0)
+        assert_raises_rpc_error(-26, NULLDUMMY_TX_ERROR, self.nodes[0].sendrawtransaction, test2tx.serialize_with_witness().hex(), 0)
 
         self.log.info(f"Test 3: Non-NULLDUMMY base transactions should be accepted in a block before activation [{COINBASE_MATURITY + 4}]")
         self.block_submit(self.nodes[0], [test2tx], accept=True)
@@ -115,7 +115,7 @@ def run_test(self):
                                           privkey=self.privkey)
         test6txs = [CTransaction(test4tx)]
         invalidate_nulldummy_tx(test4tx)
-        assert_raises_rpc_error(-26, NULLDUMMY_ERROR, self.nodes[0].sendrawtransaction, test4tx.serialize_with_witness().hex(), 0)
+        assert_raises_rpc_error(-26, NULLDUMMY_TX_ERROR, self.nodes[0].sendrawtransaction, test4tx.serialize_with_witness().hex(), 0)
         self.block_submit(self.nodes[0], [test4tx], accept=False)
 
         self.log.info("Test 5: Non-NULLDUMMY P2WSH multisig transaction invalid after activation")
@@ -125,7 +125,7 @@ def run_test(self):
                                           privkey=self.privkey)
         test6txs.append(CTransaction(test5tx))
         test5tx.wit.vtxinwit[0].scriptWitness.stack[0] = b'\x01'
-        assert_raises_rpc_error(-26, NULLDUMMY_ERROR, self.nodes[0].sendrawtransaction, test5tx.serialize_with_witness().hex(), 0)
+        assert_raises_rpc_error(-26, NULLDUMMY_TX_ERROR, self.nodes[0].sendrawtransaction, test5tx.serialize_with_witness().hex(), 0)
         self.block_submit(self.nodes[0], [test5tx], with_witness=True, accept=False)
 
         self.log.info(f"Test 6: NULLDUMMY compliant base/witness transactions should be accepted to mempool and in block after activation [{COINBASE_MATURITY + 5}]")
@@ -141,7 +141,7 @@ def block_submit(self, node, txs, *, with_witness=False, accept):
         if with_witness:
             add_witness_commitment(block)
         block.solve()
-        assert_equal(None if accept else NULLDUMMY_ERROR, node.submitblock(block.serialize().hex()))
+        assert_equal(None if accept else NULLDUMMY_BLK_ERROR, node.submitblock(block.serialize().hex()))
         if accept:
             assert_equal(node.getbestblockhash(), block.hash_hex)
             self.lastblockhash = block.hash_hex