From 2ea826cfc46ee8edfca059d0fd95ebe03122f9f2 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Wed, 29 Jul 2020 11:07:23 -0400 Subject: Add txids with non-standard inputs to reject filter Our policy checks for non-standard inputs depend only on the non-witness portion of a transaction: we look up the scriptPubKey of the input being spent from our UTXO set (which is covered by the input txid), and the p2sh checks only rely on the scriptSig portion of the input. Consequently it's safe to add txids of transactions that fail these checks to the reject filter, as the witness is irrelevant to the failure. This is helpful for any situation where we might request the transaction again via txid (either from txid-relay peers, or if we might fetch the transaction via txid due to parent-fetching of orphans). Further, in preparation for future witness versions being deployed on the network, ensure that WITNESS_UNKNOWN transactions are rejected in AreInputsStandard(), so that transactions spending v1 (or greater) witness outputs will fall into this category of having their txid added to the reject filter. Github-Pull: #19620 Rebased-From: 7989901c7eb62ca28b3d1e5d5831041a7267e495 --- src/consensus/validation.h | 4 +++- src/net_processing.cpp | 15 +++++++++++++-- src/policy/policy.cpp | 8 +++++++- src/validation.cpp | 5 +++-- 4 files changed, 26 insertions(+), 6 deletions(-) diff --git a/src/consensus/validation.h b/src/consensus/validation.h index 2e23f4b3a4..dbef958e19 100644 --- a/src/consensus/validation.h +++ b/src/consensus/validation.h @@ -48,7 +48,8 @@ enum class ValidationInvalidReason { BLOCK_TIME_FUTURE, //!< block timestamp was > 2 hours in the future (or our clock is bad) BLOCK_CHECKPOINT, //!< the block failed to meet one of our checkpoints // Only loose txn: - 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, //!< a transaction was missing some of its inputs TX_PREMATURE_SPEND, //!< transaction spends a coinbase too early, or violates locktime/sequence locks /** @@ -72,6 +73,7 @@ inline bool IsTransactionReason(ValidationInvalidReason r) return r == ValidationInvalidReason::NONE || r == ValidationInvalidReason::CONSENSUS || r == ValidationInvalidReason::RECENT_CONSENSUS_CHANGE || + r == ValidationInvalidReason::TX_INPUTS_NOT_STANDARD || r == ValidationInvalidReason::TX_NOT_STANDARD || r == ValidationInvalidReason::TX_PREMATURE_SPEND || r == ValidationInvalidReason::TX_MISSING_INPUTS || diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 3d0efa041d..4a183f0c9a 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -1055,6 +1055,7 @@ static bool MaybePunishNode(NodeId nodeid, const CValidationState& state, bool v return true; case ValidationInvalidReason::RECENT_CONSENSUS_CHANGE: case ValidationInvalidReason::BLOCK_TIME_FUTURE: + case ValidationInvalidReason::TX_INPUTS_NOT_STANDARD: case ValidationInvalidReason::TX_NOT_STANDARD: case ValidationInvalidReason::TX_MISSING_INPUTS: case ValidationInvalidReason::TX_PREMATURE_SPEND: @@ -1846,10 +1847,15 @@ void static ProcessOrphanTx(CConnman* connman, std::set& orphan_work_se // Probably non-standard or insufficient fee LogPrint(BCLog::MEMPOOL, " removed orphan tx %s\n", orphanHash.ToString()); assert(IsTransactionReason(orphan_state.GetReason())); - if (!orphanTx.HasWitness() && orphan_state.GetReason() != ValidationInvalidReason::TX_WITNESS_MUTATED) { + if ((!orphanTx.HasWitness() && orphan_state.GetReason() != ValidationInvalidReason::TX_WITNESS_MUTATED) || + orphan_state.GetReason() == ValidationInvalidReason::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(orphanHash); } @@ -2574,10 +2580,15 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr } } else { assert(IsTransactionReason(state.GetReason())); - if (!tx.HasWitness() && state.GetReason() != ValidationInvalidReason::TX_WITNESS_MUTATED) { + if ((!tx.HasWitness() && state.GetReason() != ValidationInvalidReason::TX_WITNESS_MUTATED) || + state.GetReason() == ValidationInvalidReason::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 51de5841ec..636a916602 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 ac98bd61c7..bdf3c128cc 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -678,8 +678,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(ValidationInvalidReason::TX_NOT_STANDARD, false, REJECT_NONSTANDARD, "bad-txns-nonstandard-inputs"); + if (fRequireStandard && !AreInputsStandard(tx, m_view)) { + return state.Invalid(ValidationInvalidReason::TX_INPUTS_NOT_STANDARD, false, REJECT_NONSTANDARD, "bad-txns-nonstandard-inputs"); + } // Check for non-standard witness in P2WSH if (tx.HasWitness() && fRequireStandard && !IsWitnessStandard(tx, m_view)) -- cgit v1.2.3 From 52c3bec1bad4fc602a3911b44de48c6b0dbdfd25 Mon Sep 17 00:00:00 2001 From: Gregory Sanders Date: Mon, 3 Aug 2020 14:37:58 -0400 Subject: test addition of unknown segwit spends to txid reject filter Github-Pull: #19620 Rebased-From: 9f88ded82b2898ca63d44c08072f1ba52f0e18d7 --- test/functional/p2p_segwit.py | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/test/functional/p2p_segwit.py b/test/functional/p2p_segwit.py index 98f6b1d71d..760cedde3a 100755 --- a/test/functional/p2p_segwit.py +++ b/test/functional/p2p_segwit.py @@ -1391,7 +1391,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: @@ -1399,8 +1399,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") -- cgit v1.2.3