diff --git a/src/consensus/validation.h b/src/consensus/validation.h index a79e7b9d127..3a90cd69b32 100644 --- a/src/consensus/validation.h +++ b/src/consensus/validation.h @@ -26,7 +26,8 @@ enum class TxValidationResult { * is uninteresting. */ TX_RECENT_CONSENSUS_CHANGE, - TX_NOT_STANDARD, //!< didn't meet our local policy rules + TX_INPUTS_NOT_STANDARD, //!< inputs (covered by txid) failed policy rules + TX_NOT_STANDARD, //!< otherwise didn't meet our local policy rules TX_MISSING_INPUTS, //!< transaction was missing some of its inputs TX_PREMATURE_SPEND, //!< transaction spends a coinbase too early, or violates locktime/sequence locks /** diff --git a/src/net_processing.cpp b/src/net_processing.cpp index ddc516ce0f8..8572ebb9f72 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1069,6 +1069,7 @@ static bool MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state, } // Conflicting (but not necessarily invalid) data or different policy: case TxValidationResult::TX_RECENT_CONSENSUS_CHANGE: + case TxValidationResult::TX_INPUTS_NOT_STANDARD: case TxValidationResult::TX_NOT_STANDARD: case TxValidationResult::TX_MISSING_INPUTS: case TxValidationResult::TX_PREMATURE_SPEND: @@ -1901,10 +1902,15 @@ void static ProcessOrphanTx(CConnman* connman, CTxMemPool& mempool, std::setinsert(orphanHash); } @@ -2596,10 +2602,15 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec recentRejects->insert(tx.GetHash()); } } else { - if (!tx.HasWitness() && state.GetResult() != TxValidationResult::TX_WITNESS_MUTATED) { + if ((!tx.HasWitness() && state.GetResult() != TxValidationResult::TX_WITNESS_MUTATED) || + state.GetResult() == TxValidationResult::TX_INPUTS_NOT_STANDARD) { // Do not use rejection cache for witness transactions or // witness-stripped transactions, as they can have been malleated. // See https://github.com/bitcoin/bitcoin/issues/8279 for details. + // However, if the transaction failed for TX_INPUTS_NOT_STANDARD, + // then we know that the witness was irrelevant to the policy + // failure, since this check depends only on the txid + // (the scriptPubKey being spent is covered by the txid). assert(recentRejects); recentRejects->insert(tx.GetHash()); if (RecursiveDynamicUsage(*ptx) < 100000) { diff --git a/src/policy/policy.cpp b/src/policy/policy.cpp index 07d51c00887..a0ac6bdd830 100644 --- a/src/policy/policy.cpp +++ b/src/policy/policy.cpp @@ -152,6 +152,8 @@ bool IsStandardTx(const CTransaction& tx, bool permit_bare_multisig, const CFeeR * script can be anything; an attacker could use a very * expensive-to-check-upon-redemption script like: * DUP CHECKSIG DROP ... repeated 100 times... OP_1 + * + * Note that only the non-witness portion of the transaction is checked here. */ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs) { @@ -164,7 +166,11 @@ bool AreInputsStandard(const CTransaction& tx, const CCoinsViewCache& mapInputs) std::vector > vSolutions; txnouttype whichType = Solver(prev.scriptPubKey, vSolutions); - if (whichType == TX_NONSTANDARD) { + if (whichType == TX_NONSTANDARD || whichType == TX_WITNESS_UNKNOWN) { + // WITNESS_UNKNOWN failures are typically also caught with a policy + // flag in the script interpreter, but it can be helpful to catch + // this type of NONSTANDARD transaction earlier in transaction + // validation. return false; } else if (whichType == TX_SCRIPTHASH) { std::vector > stack; diff --git a/src/validation.cpp b/src/validation.cpp index 7ee94f86575..5a98b2cb927 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -665,8 +665,9 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws) } // Check for non-standard pay-to-script-hash in inputs - if (fRequireStandard && !AreInputsStandard(tx, m_view)) - return state.Invalid(TxValidationResult::TX_NOT_STANDARD, "bad-txns-nonstandard-inputs"); + if (fRequireStandard && !AreInputsStandard(tx, m_view)) { + return state.Invalid(TxValidationResult::TX_INPUTS_NOT_STANDARD, "bad-txns-nonstandard-inputs"); + } // Check for non-standard witness in P2WSH if (tx.HasWitness() && fRequireStandard && !IsWitnessStandard(tx, m_view)) diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index d8dce7fe56e..a7cfefc4857 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -1393,7 +1393,7 @@ class SegWitTest(BitcoinTestFramework): temp_utxo.pop() # last entry in temp_utxo was the output we just spent temp_utxo.append(UTXO(tx2.sha256, 0, tx2.vout[0].nValue)) - # Spend everything in temp_utxo back to an OP_TRUE output. + # Spend everything in temp_utxo into an segwit v1 output. tx3 = CTransaction() total_value = 0 for i in temp_utxo: @@ -1401,8 +1401,16 @@ class SegWitTest(BitcoinTestFramework): tx3.wit.vtxinwit.append(CTxInWitness()) total_value += i.nValue tx3.wit.vtxinwit[-1].scriptWitness.stack = [witness_program] - tx3.vout.append(CTxOut(total_value - 1000, CScript([OP_TRUE]))) + tx3.vout.append(CTxOut(total_value - 1000, script_pubkey)) tx3.rehash() + + # First we test this transaction against fRequireStandard=true node + # making sure the txid is added to the reject filter + self.std_node.announce_tx_and_wait_for_getdata(tx3) + test_transaction_acceptance(self.nodes[1], self.std_node, tx3, with_witness=True, accepted=False, reason="bad-txns-nonstandard-inputs") + # Now the node will no longer ask for getdata of this transaction when advertised by same txid + self.std_node.announce_tx_and_wait_for_getdata(tx3, timeout=5, success=False) + # Spending a higher version witness output is not allowed by policy, # even with fRequireStandard=false. test_transaction_acceptance(self.nodes[0], self.test_node, tx3, with_witness=True, accepted=False, reason="reserved for soft-fork upgrades")