Merge #17985: net: Remove forcerelay of rejected txs

facb71576cd4d2e90fd03e09d29b42fa3d730e8c net: Remove forcerelay of rejected txs (MarcoFalke)

Pull request description:

  This removes the code that supposedly handled the forced relay of txs from a permissioned peer that were rejected from our mempool. The removal should be fine, because it is dead code for the following reasons:

  * While `RelayTransaction` enqueues the inv for all peers, the inv is never processed because it can not be found in the mempool. See https://github.com/bitcoin/bitcoin/blob/4a072330763b3ff2d1b5c5b8d30a9517873ac6de/src/net_processing.cpp#L3862-L3866

  * Even if the peers we intended to send the inv to can somehow reply with a getdata to the never-received inv, they won't receive the tx as a reply because it was never added to the "relay memory" (`mapRelay`)

  The dead code is (obviously) untested: https://marcofalke.github.io/btc_cov/total.coverage/src/net_processing.cpp.gcov.html#2574

  This feature was (intentionally or accidentally) removed in 4d8993b3469915d8c9ba4cd3b918f16782edf0de, which was released in Bitcoin Core 0.13.0. So all currently supported versions of Bitcoin Core ship without this feature. I am not aware of any complaints about this feature or actual documented use-cases. So instead of reviving an unneeded feature, just remove the dead code.

ACKs for top commit:
  hebasto:
    ACK facb71576cd4d2e90fd03e09d29b42fa3d730e8c, locally running the unit and functional tests.

Tree-SHA512: bfceae6f2983c1510fa0649a9a63c343cbbc1c4ab3a3698039cccf454c81e58c8f5114b147ed42a1bc867da74c43a5b53764ab14f942e191b6f59079044108b5

4 files changed, 20 insertions, 28 deletions

diff --git a/src/init.cpp b/src/init.cpp
index 24a1fd27db..ece6214aa8 100644
--- a/src/init.cpp
+++ b/src/init.cpp
@@ -450,7 +450,7 @@ void SetupServerArgs()
     gArgs.AddArg("-whitebind=<[permissions@]addr>", "Bind to given address and whitelist peers connecting to it. "
         "Use [host]:port notation for IPv6. Allowed permissions are bloomfilter (allow requesting BIP37 filtered blocks and transactions), "
         "noban (do not ban for misbehavior), "
-        "forcerelay (relay even non-standard transactions), "
+        "forcerelay (relay transactions that are already in the mempool; implies relay), "
         "relay (relay even in -blocksonly mode), "
         "and mempool (allow requesting BIP35 mempool contents). "
         "Specify multiple permissions separated by commas (default: noban,mempool,relay). Can be specified multiple times.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
@@ -527,7 +527,7 @@ void SetupServerArgs()
     gArgs.AddArg("-datacarriersize", strprintf("Maximum size of data in data carrier transactions we relay and mine (default: %u)", MAX_OP_RETURN_RELAY), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
     gArgs.AddArg("-minrelaytxfee=<amt>", strprintf("Fees (in %s/kB) smaller than this are considered zero fee for relaying, mining and transaction creation (default: %s)",
         CURRENCY_UNIT, FormatMoney(DEFAULT_MIN_RELAY_TX_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
-    gArgs.AddArg("-whitelistforcerelay", strprintf("Add 'forcerelay' permission to whitelisted inbound peers with default permissions. This will relay transactions even if the transactions were already in the mempool or violate local relay policy. (default: %d)", DEFAULT_WHITELISTFORCERELAY), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
+    gArgs.AddArg("-whitelistforcerelay", strprintf("Add 'forcerelay' permission to whitelisted inbound peers with default permissions. This will relay transactions even if the transactions were already in the mempool. (default: %d)", DEFAULT_WHITELISTFORCERELAY), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
     gArgs.AddArg("-whitelistrelay", strprintf("Add 'relay' permission to whitelisted inbound peers with default permissions. This will accept relayed transactions even when not relaying transactions (default: %d)", DEFAULT_WHITELISTRELAY), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
 
 
diff --git a/src/net_permissions.h b/src/net_permissions.h
index a06d2f544d..ad74848347 100644
--- a/src/net_permissions.h
+++ b/src/net_permissions.h
@@ -15,7 +15,7 @@ enum NetPermissionFlags
     PF_BLOOMFILTER = (1U << 1),
     // Relay and accept transactions from this peer, even if -blocksonly is true
     PF_RELAY = (1U << 3),
-    // Always relay transactions from this peer, even if already in mempool or rejected from policy
+    // Always relay transactions from this peer, even if already in mempool
     // Keep parameter interaction: forcerelay implies relay
     PF_FORCERELAY = (1U << 2) | PF_RELAY,
     // Can't be banned for misbehavior
@@ -59,4 +59,4 @@ public:
     CSubNet m_subnet;
 };
 
-#endif // BITCOIN_NET_PERMISSIONS_H
\ No newline at end of file
+#endif // BITCOIN_NET_PERMISSIONS_H
diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index cf4aee0647..1c1046b6ff 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -987,15 +987,6 @@ void Misbehaving(NodeId pnode, int howmuch, const std::string& message) EXCLUSIV
 }
 
 /**
- * Returns true if the given validation state result may result in a peer
- * banning/disconnecting us. We use this to determine which unaccepted
- * transactions from a whitelisted peer that we can safely relay.
- */
-static bool TxRelayMayResultInDisconnect(const TxValidationState& state) {
-    return state.GetResult() == TxValidationResult::TX_CONSENSUS;
-}
-
-/**
  * Potentially ban a node based on the contents of a BlockValidationState object
  *
  * @param[in] via_compact_block this bool is passed in because net_processing should
@@ -1064,10 +1055,9 @@ static bool MaybePunishNodeForBlock(NodeId nodeid, const BlockValidationState& s
  * Potentially ban a node based on the contents of a TxValidationState object
  *
  * @return Returns true if the peer was punished (probably disconnected)
- *
- * Changes here may need to be reflected in TxRelayMayResultInDisconnect().
  */
-static bool MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state, const std::string& message = "") {
+static bool MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state, const std::string& message = "")
+{
     switch (state.GetResult()) {
     case TxValidationResult::TX_RESULT_UNSET:
         break;
@@ -1095,11 +1085,6 @@ static bool MaybePunishNodeForTx(NodeId nodeid, const TxValidationState& state,
 }
 
 
-
-
-
-
-
 //////////////////////////////////////////////////////////////////////////////
 //
 // blockchain -> download logic notification
@@ -2615,14 +2600,11 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
 
             if (pfrom->HasPermission(PF_FORCERELAY)) {
                 // Always relay transactions received from whitelisted peers, even
-                // if they were already in the mempool or rejected from it due
-                // to policy, allowing the node to function as a gateway for
+                // if they were already in the mempool,
+                // allowing the node to function as a gateway for
                 // nodes hidden behind it.
-                //
-                // Never relay transactions that might result in being
-                // disconnected (or banned).
-                if (state.IsInvalid() && TxRelayMayResultInDisconnect(state)) {
-                    LogPrintf("Not relaying invalid transaction %s from whitelisted peer=%d (%s)\n", tx.GetHash().ToString(), pfrom->GetId(), FormatStateMessage(state));
+                if (!mempool.exists(tx.GetHash())) {
+                    LogPrintf("Not relaying non-mempool transaction %s from whitelisted peer=%d\n", tx.GetHash().ToString(), pfrom->GetId());
                 } else {
                     LogPrintf("Force relaying tx %s from whitelisted peer=%d\n", tx.GetHash().ToString(), pfrom->GetId());
                     RelayTransaction(tx.GetHash(), *connman);
diff --git a/test/functional/p2p_permissions.py b/test/functional/p2p_permissions.py
index 93e2957fd0..3a7bf4bfc3 100755
--- a/test/functional/p2p_permissions.py
+++ b/test/functional/p2p_permissions.py
@@ -132,6 +132,16 @@ class P2PPermissionsTests(BitcoinTestFramework):
             p2p_rebroadcast_wallet.send_txs_and_test([tx], self.nodes[1])
             wait_until(lambda: txid in self.nodes[0].getrawmempool())
 
+        self.log.debug("Check that node[1] will not send an invalid tx to node[0]")
+        tx.vout[0].nValue += 1
+        txid = tx.rehash()
+        p2p_rebroadcast_wallet.send_txs_and_test(
+            [tx],
+            self.nodes[1],
+            success=False,
+            reject_reason='Not relaying non-mempool transaction {} from whitelisted peer=0'.format(txid),
+        )
+
     def checkpermission(self, args, expectedPermissions, whitelisted):
         self.restart_node(1, args)
         connect_nodes(self.nodes[0], 1)