foster/bitcoin-bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-08 10:31:50 -05:00
Code Issues Activity

Make TX_WITNESS_STRIPPED its own rejection reason

Browse source 
Previously, TX_WITNESS_MUTATED could be returned during transaction validation
for either transactions that had a witness that was non-standard, or for
transactions that had no witness but were invalid due to segwit validation
rules.

However, for txid/wtxid-relay considerations, net_processing distinguishes the
witness stripped case separately, because it affects whether a wtxid should be
able to be added to the reject filter. It is safe to add the wtxid of a
witness-mutated transaction to the filter (as that wtxid shouldn't collide with
the txid, and hence it wouldn't interfere with transaction relay from
txid-relay peers), but it is not safe to add the wtxid (== txid) of a
witness-stripped transaction to the filter, because that would interfere with
relay of another transaction with the same txid (but different wtxid) when
relaying from txid-relay peers.

Also updates the comment explaining this logic, and explaining that we can get
rid of this complexity once there's a sufficient deployment of wtxid-relaying
peers on the network.
This commit is contained in:
Suhas Daftuar 2020-02-07 04:30:41 -05:00
parent 97141ca442
commit 4eb515574e
3 changed files with 33 additions and 10 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
src/consensus/validation.h
View file

@ -30,11 +30,15 @@ enum class TxValidationResult {
TX_MISSING_INPUTS, //!< transaction was missing some of its inputs
TX_PREMATURE_SPEND, //!< transaction spends a coinbase too early, or violates locktime/sequence locks
/**
* Transaction might be missing a witness, have a witness prior to SegWit
* Transaction might have a witness prior to SegWit
* activation, or witness may have been malleated (which includes
* non-standard witnesses).
*/
TX_WITNESS_MUTATED,
/**
* Transaction is missing a witness.
*/
TX_WITNESS_STRIPPED,
/**
* Tx already in mempool or conflicts with a tx in the chain
* (if it conflicts with another tx in mempool, we use MEMPOOL_POLICY as it failed to reach the RBF threshold)

35
src/net_processing.cpp
View file

@ -1160,6 +1160,7 @@ static bool MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state,
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:
break;
@ -2031,10 +2032,19 @@ void static ProcessOrphanTx(CConnman& connman, CTxMemPool& mempool, std::set<uin
// Has inputs but not accepted to mempool
// Probably non-standard or insufficient fee
LogPrint(BCLog::MEMPOOL, " removed orphan tx %s\n", orphanHash.ToString());
if (orphanTx.HasWitness() || orphan_state.GetResult() != TxValidationResult::TX_WITNESS_MUTATED) {
// 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.
if (orphan_state.GetResult() != TxValidationResult::TX_WITNESS_STRIPPED) {
// Do not add txids of witness transactions or witness-stripped
// transactions to the filter, as they can have been malleated;
// adding such txids to the reject filter would potentially
// interfere with relay of valid transactions from peers that
// do not support wtxid-based relay. See
// https://github.com/bitcoin/bitcoin/issues/8279 for details.
// We can remove this restriction (and always add wtxids to
// the filter even for witness stripped transactions) once
// wtxid-based relay is broadly deployed.
// See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034
// for concerns around weakening security of unupgraded nodes
// if we start doing this too early.
assert(recentRejects);
recentRejects->insert(orphanTx.GetWitnessHash());
}
@ -2992,10 +3002,19 @@ void ProcessMessage(
recentRejects->insert(tx.GetWitnessHash());
}
} else {
if (tx.HasWitness() || state.GetResult() != TxValidationResult::TX_WITNESS_MUTATED) {
// 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.
if (state.GetResult() != TxValidationResult::TX_WITNESS_STRIPPED) {
// Do not add txids of witness transactions or witness-stripped
// transactions to the filter, as they can have been malleated;
// adding such txids to the reject filter would potentially
// interfere with relay of valid transactions from peers that
// do not support wtxid-based relay. See
// https://github.com/bitcoin/bitcoin/issues/8279 for details.
// We can remove this restriction (and always add wtxids to
// the filter even for witness stripped transactions) once
// wtxid-based relay is broadly deployed.
// See also comments in https://github.com/bitcoin/bitcoin/pull/18044#discussion_r443419034
// for concerns around weakening security of unupgraded nodes
// if we start doing this too early.
assert(recentRejects);
recentRejects->insert(tx.GetWitnessHash());
if (RecursiveDynamicUsage(*ptx) < 100000) {

2
src/validation.cpp
View file

@ -939,7 +939,7 @@ bool MemPoolAccept::PolicyScriptChecks(ATMPArgs& args, Workspace& ws, Precompute
if (!tx.HasWitness() && CheckInputScripts(tx, state_dummy, m_view, scriptVerifyFlags & ~(SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_CLEANSTACK), true, false, txdata) &&
!CheckInputScripts(tx, state_dummy, m_view, scriptVerifyFlags & ~SCRIPT_VERIFY_CLEANSTACK, true, false, txdata)) {
// Only the witness is missing, so the transaction itself may be fine.
state.Invalid(TxValidationResult::TX_WITNESS_MUTATED,
state.Invalid(TxValidationResult::TX_WITNESS_STRIPPED,
state.GetRejectReason(), state.GetDebugMessage());
}
return false; // state filled in by CheckInputScripts