Merge bitcoin#20399: Revert "Merge bitcoin#19606: Backport wtxid rela…

…y to v0.20"

fa074d2 Revert "Merge bitcoin#19606: Backport wtxid relay to v0.20" (MarcoFalke)

Pull request description:

  The 0.20 branch has bugfixes that should be released. However, a tag can currently not be created because the latest merge introduced a regression and is not a bugfix (bitcoin#20317 (comment), bitcoin#20317 (comment)).

  Fix that by reverting the last merge. Can be reviewed by re-doing the revert or calling `git diff HEAD HEAD~2 | wc` and observing an empty diff.

ACKs for top commit:
  laanwj:
    Code review ACK fa074d2

Tree-SHA512: 1a1314b9bb85f44696dc307845e80292998d6c9c000e7386c48405e74400d9cd22be6996e555f198da917e04024a1c8e609dfd830759a27fe4070168b0d272bb

src/consensus/validation.h

@@ -31,15 +31,11 @@ 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 have a witness prior to SegWit
+     * Transaction might be missing a witness, 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)

src/net.h

@@ -969,11 +969,11 @@ class CNode
     }
 
 
-    void AddKnownTx(const uint256& hash)
+    void AddInventoryKnown(const CInv& inv)
     {
         if (m_tx_relay != nullptr) {
             LOCK(m_tx_relay->cs_tx_inventory);
-            m_tx_relay->filterInventoryKnown.insert(hash);
+            m_tx_relay->filterInventoryKnown.insert(inv.hash);
         }
     }
 

src/net_processing.h

@@ -92,6 +92,6 @@ struct CNodeStateStats {
 bool GetNodeStateStats(NodeId nodeid, CNodeStateStats &stats);
 
 /** Relay transaction to every node */
-void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman& connman) EXCLUSIVE_LOCKS_REQUIRED(cs_main);
+void RelayTransaction(const uint256&, const CConnman& connman);
 
 #endif // BITCOIN_NET_PROCESSING_H

src/node/transaction.cpp

@@ -78,8 +78,7 @@ TransactionError BroadcastTransaction(NodeContext& node, const CTransactionRef t
     }
 
     if (relay) {
-        LOCK(cs_main);
-        RelayTransaction(hashTx, tx->GetWitnessHash(), *node.connman);
+        RelayTransaction(hashTx, *node.connman);
     }
 
     return TransactionError::OK;

src/protocol.cpp

@@ -40,7 +40,6 @@ const char *SENDCMPCT="sendcmpct";
 const char *CMPCTBLOCK="cmpctblock";
 const char *GETBLOCKTXN="getblocktxn";
 const char *BLOCKTXN="blocktxn";
-const char *WTXIDRELAY="wtxidrelay";
 } // namespace NetMsgType
 
 /** All known message types. Keep this in the same order as the list of
@@ -72,7 +71,6 @@ const static std::string allNetMessageTypes[] = {
     NetMsgType::CMPCTBLOCK,
     NetMsgType::GETBLOCKTXN,
     NetMsgType::BLOCKTXN,
-    NetMsgType::WTXIDRELAY,
 };
 const static std::vector<std::string> allNetMessageTypesVec(allNetMessageTypes, allNetMessageTypes+ARRAYLEN(allNetMessageTypes));
 
@@ -185,8 +183,6 @@ std::string CInv::GetCommand() const
     switch (masked)
     {
     case MSG_TX:             return cmd.append(NetMsgType::TX);
-    // WTX is not a message type, just an inv type
-    case MSG_WTX:            return cmd.append("wtx");
     case MSG_BLOCK:          return cmd.append(NetMsgType::BLOCK);
     case MSG_FILTERED_BLOCK: return cmd.append(NetMsgType::MERKLEBLOCK);
     case MSG_CMPCT_BLOCK:    return cmd.append(NetMsgType::CMPCTBLOCK);

src/protocol.h

@@ -234,12 +234,6 @@ extern const char *GETBLOCKTXN;
  * @since protocol version 70014 as described by BIP 152
  */
 extern const char *BLOCKTXN;
-/**
- * Indicates that a node prefers to relay transactions via wtxid, rather than
- * txid.
- * @since protocol version 70016 as described by BIP 339.
- */
-extern const char *WTXIDRELAY;
 };
 
 /* Get a vector of all valid message types (see above) */
@@ -368,12 +362,12 @@ const uint32_t MSG_TYPE_MASK    = 0xffffffff >> 2;
  * These numbers are defined by the protocol. When adding a new value, be sure
  * to mention it in the respective BIP.
  */
-enum GetDataMsg : uint32_t {
+enum GetDataMsg
+{
     UNDEFINED = 0,
     MSG_TX = 1,
     MSG_BLOCK = 2,
-    MSG_WTX = 5,                                      //!< Defined in BIP 339
-    // The following can only occur in getdata. Invs always use TX/WTX or BLOCK.
+    // The following can only occur in getdata. Invs always use TX or BLOCK.
     MSG_FILTERED_BLOCK = 3,  //!< Defined in BIP37
     MSG_CMPCT_BLOCK = 4,     //!< Defined in BIP152
     MSG_WITNESS_BLOCK = MSG_BLOCK | MSG_WITNESS_FLAG, //!< Defined in BIP144

src/txmempool.cpp

@@ -724,12 +724,12 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const
     assert(innerUsage == cachedInnerUsage);
 }
 
-bool CTxMemPool::CompareDepthAndScore(const uint256& hasha, const uint256& hashb, bool wtxid)
+bool CTxMemPool::CompareDepthAndScore(const uint256& hasha, const uint256& hashb)
 {
     LOCK(cs);
-    indexed_transaction_set::const_iterator i = wtxid ? get_iter_from_wtxid(hasha) : mapTx.find(hasha);
+    indexed_transaction_set::const_iterator i = mapTx.find(hasha);
     if (i == mapTx.end()) return false;
-    indexed_transaction_set::const_iterator j = wtxid ? get_iter_from_wtxid(hashb) : mapTx.find(hashb);
+    indexed_transaction_set::const_iterator j = mapTx.find(hashb);
     if (j == mapTx.end()) return true;
     uint64_t counta = i->GetCountWithAncestors();
     uint64_t countb = j->GetCountWithAncestors();
@@ -809,10 +809,10 @@ CTransactionRef CTxMemPool::get(const uint256& hash) const
     return i->GetSharedTx();
 }
 
-TxMempoolInfo CTxMemPool::info(const uint256& hash, bool wtxid) const
+TxMempoolInfo CTxMemPool::info(const uint256& hash) const
 {
     LOCK(cs);
-    indexed_transaction_set::const_iterator i = (wtxid ? get_iter_from_wtxid(hash) : mapTx.find(hash));
+    indexed_transaction_set::const_iterator i = mapTx.find(hash);
     if (i == mapTx.end())
         return TxMempoolInfo();
     return GetInfo(i);
@@ -915,8 +915,8 @@ bool CCoinsViewMemPool::GetCoin(const COutPoint &outpoint, Coin &coin) const {
 
 size_t CTxMemPool::DynamicMemoryUsage() const {
     LOCK(cs);
-    // Estimate the overhead of mapTx to be 15 pointers + an allocation, as no exact formula for boost::multi_index_contained is implemented.
-    return memusage::MallocUsage(sizeof(CTxMemPoolEntry) + 15 * sizeof(void*)) * mapTx.size() + memusage::DynamicUsage(mapNextTx) + memusage::DynamicUsage(mapDeltas) + memusage::DynamicUsage(mapLinks) + memusage::DynamicUsage(vTxHashes) + cachedInnerUsage;
+    // Estimate the overhead of mapTx to be 12 pointers + an allocation, as no exact formula for boost::multi_index_contained is implemented.
+    return memusage::MallocUsage(sizeof(CTxMemPoolEntry) + 12 * sizeof(void*)) * mapTx.size() + memusage::DynamicUsage(mapNextTx) + memusage::DynamicUsage(mapDeltas) + memusage::DynamicUsage(mapLinks) + memusage::DynamicUsage(vTxHashes) + cachedInnerUsage;
 }
 
 void CTxMemPool::RemoveStaged(setEntries &stage, bool updateDescendants, MemPoolRemovalReason reason) {

src/txmempool.h

@@ -198,22 +198,6 @@ struct mempoolentry_txid
     }
 };
 
-// extracts a transaction witness-hash from CTxMemPoolEntry or CTransactionRef
-struct mempoolentry_wtxid
-{
-    typedef uint256 result_type;
-    result_type operator() (const CTxMemPoolEntry &entry) const
-    {
-        return entry.GetTx().GetWitnessHash();
-    }
-
-    result_type operator() (const CTransactionRef& tx) const
-    {
-        return tx->GetWitnessHash();
-    }
-};
-
-
 /** \class CompareTxMemPoolEntryByDescendantScore
  *
  *  Sort an entry by max(score/size of entry's tx, score/size with all descendants).
@@ -334,7 +318,6 @@ class CompareTxMemPoolEntryByAncestorFee
 struct descendant_score {};
 struct entry_time {};
 struct ancestor_score {};
-struct index_by_wtxid {};
 
 class CBlockPolicyEstimator;
 
@@ -400,9 +383,8 @@ class SaltedTxidHasher
  *
  * CTxMemPool::mapTx, and CTxMemPoolEntry bookkeeping:
  *
- * mapTx is a boost::multi_index that sorts the mempool on 5 criteria:
- * - transaction hash (txid)
- * - witness-transaction hash (wtxid)
+ * mapTx is a boost::multi_index that sorts the mempool on 4 criteria:
+ * - transaction hash
  * - descendant feerate [we use max(feerate of tx, feerate of tx with all descendants)]
  * - time in mempool
  * - ancestor feerate [we use min(feerate of tx, feerate of tx with all unconfirmed ancestors)]
@@ -487,12 +469,6 @@ class CTxMemPool
         boost::multi_index::indexed_by<
             // sorted by txid
             boost::multi_index::hashed_unique<mempoolentry_txid, SaltedTxidHasher>,
-            // sorted by wtxid
-            boost::multi_index::hashed_unique<
-                boost::multi_index::tag<index_by_wtxid>,
-                mempoolentry_wtxid,
-                SaltedTxidHasher
-            >,
             // sorted by fee rate
             boost::multi_index::ordered_non_unique<
                 boost::multi_index::tag<descendant_score>,
@@ -607,7 +583,7 @@ class CTxMemPool
 
     void clear();
     void _clear() EXCLUSIVE_LOCKS_REQUIRED(cs); //lock free
-    bool CompareDepthAndScore(const uint256& hasha, const uint256& hashb, bool wtxid=false);
+    bool CompareDepthAndScore(const uint256& hasha, const uint256& hashb);
     void queryHashes(std::vector<uint256>& vtxid) const;
     bool isSpent(const COutPoint& outpoint) const;
     unsigned int GetTransactionsUpdated() const;
@@ -710,22 +686,14 @@ class CTxMemPool
         return totalTxSize;
     }
 
-    bool exists(const uint256& hash, bool wtxid=false) const
+    bool exists(const uint256& hash) const
     {
         LOCK(cs);
-        if (wtxid) {
-            return (mapTx.get<index_by_wtxid>().count(hash) != 0);
-        }
         return (mapTx.count(hash) != 0);
     }
 
     CTransactionRef get(const uint256& hash) const;
-    txiter get_iter_from_wtxid(const uint256& wtxid) const EXCLUSIVE_LOCKS_REQUIRED(cs)
-    {
-        AssertLockHeld(cs);
-        return mapTx.project<0>(mapTx.get<index_by_wtxid>().find(wtxid));
-    }
-    TxMempoolInfo info(const uint256& hash, bool wtxid=false) const;
+    TxMempoolInfo info(const uint256& hash) const;
     std::vector<TxMempoolInfo> infoAll() const;
 
     size_t DynamicMemoryUsage() const;

src/validation.cpp

@@ -916,7 +916,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_STRIPPED,
+            state.Invalid(TxValidationResult::TX_WITNESS_MUTATED,
                     state.GetRejectReason(), state.GetDebugMessage());
         }
         return false; // state filled in by CheckInputScripts

src/version.h

@@ -9,7 +9,7 @@
  * network protocol versioning
  */
 
-static const int PROTOCOL_VERSION = 70016;
+static const int PROTOCOL_VERSION = 70015;
 
 //! initial proto version, to be increased after version/verack negotiation
 static const int INIT_PROTO_VERSION = 209;
@@ -42,7 +42,4 @@ static const int SHORT_IDS_BLOCKS_VERSION = 70014;
 //! not banning for invalid compact blocks starts with this version
 static const int INVALID_CB_NO_BAN_VERSION = 70015;
 
-//! "wtxidrelay" command for wtxid-based relay starts with this version
-static const int WTXID_RELAY_VERSION = 70016;
-
 #endif // BITCOIN_VERSION_H

test/functional/mempool_packages.py

@@ -67,15 +67,10 @@ def run_test(self):
         fee = Decimal("0.0001")
         # MAX_ANCESTORS transactions off a confirmed tx should be fine
         chain = []
-        witness_chain = []
         for i in range(MAX_ANCESTORS):
             (txid, sent_value) = self.chain_transaction(self.nodes[0], txid, 0, value, fee, 1)
             value = sent_value
             chain.append(txid)
-            # We need the wtxids to check P2P announcements
-            fulltx = self.nodes[0].getrawtransaction(txid)
-            witnesstx = self.nodes[0].decoderawtransaction(fulltx, True)
-            witness_chain.append(witnesstx['hash'])
 
         # Check mempool has MAX_ANCESTORS transactions in it, and descendant and ancestor
         # count and fees should look correct

test/functional/p2p_blocksonly.py

@@ -52,7 +52,7 @@ def run_test(self):
         self.log.info('Check that txs from rpc are not rejected and relayed to other peers')
         assert_equal(self.nodes[0].getpeerinfo()[0]['relaytxes'], True)
         txid = self.nodes[0].testmempoolaccept([sigtx])[0]['txid']
-        with self.nodes[0].assert_debug_log(['received getdata for: wtx {} peer=1'.format(txid)]):
+        with self.nodes[0].assert_debug_log(['received getdata for: tx {} peer=1'.format(txid)]):
             self.nodes[0].sendrawtransaction(sigtx)
             self.nodes[0].p2p.wait_for_tx(txid)
             assert_equal(self.nodes[0].getmempoolinfo()['size'], 1)

test/functional/p2p_feefilter.py

@@ -7,7 +7,7 @@
 from decimal import Decimal
 import time
 
-from test_framework.messages import MSG_TX, MSG_WTX, msg_feefilter
+from test_framework.messages import msg_feefilter
 from test_framework.mininode import mininode_lock, P2PInterface
 from test_framework.test_framework import BitcoinTestFramework
 
@@ -31,7 +31,7 @@ def __init__(self):
 
     def on_inv(self, message):
         for i in message.inv:
-            if (i.type == MSG_TX) or (i.type == MSG_WTX):
+            if (i.type == 1):
                 self.txinvs.append(hashToHex(i.hash))
 
     def clear_invs(self):