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 4a07233076/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 4d8993b346, 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
This commit is contained in:
Wladimir J. van der Laan 2020-02-26 18:44:25 +01:00 committed by munkybooty
parent 4b4856ee80
commit b0518f0796
4 changed files with 19 additions and 30 deletions

View File

@ -608,7 +608,7 @@ void SetupServerArgs(NodeContext& node)
argsman.AddArg("-whitebind=<[permissions@]addr>", "Bind to given address and whitelist peers connecting to it. " argsman.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), " "Use [host]:port notation for IPv6. Allowed permissions are bloomfilter (allow requesting BIP37 filtered blocks and transactions), "
"noban (do not ban for misbehavior), " "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), " "relay (relay even in -blocksonly mode), "
"and mempool (allow requesting BIP35 mempool contents). " "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); "Specify multiple permissions separated by commas (default: noban,mempool,relay). Can be specified multiple times.", ArgsManager::ALLOW_ANY, OptionsCategory::CONNECTION);
@ -753,7 +753,7 @@ void SetupServerArgs(NodeContext& node)
argsman.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); argsman.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);
argsman.AddArg("-minrelaytxfee=<amt>", strprintf("Fees (in %s/kB) smaller than this are considered zero fee for relaying, mining and transaction creation (default: %s)", argsman.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); CURRENCY_UNIT, FormatMoney(DEFAULT_MIN_RELAY_TX_FEE)), ArgsManager::ALLOW_ANY, OptionsCategory::NODE_RELAY);
argsman.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); argsman.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);
argsman.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); argsman.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);
argsman.AddArg("-blockmaxsize=<n>", strprintf("Set maximum block size in bytes (default: %d)", DEFAULT_BLOCK_MAX_SIZE), ArgsManager::ALLOW_ANY, OptionsCategory::BLOCK_CREATION); argsman.AddArg("-blockmaxsize=<n>", strprintf("Set maximum block size in bytes (default: %d)", DEFAULT_BLOCK_MAX_SIZE), ArgsManager::ALLOW_ANY, OptionsCategory::BLOCK_CREATION);

View File

@ -18,7 +18,7 @@ enum NetPermissionFlags
PF_BLOOMFILTER = (1U << 1), PF_BLOOMFILTER = (1U << 1),
// Relay and accept transactions from this peer, even if -blocksonly is true // Relay and accept transactions from this peer, even if -blocksonly is true
PF_RELAY = (1U << 3), 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 // Keep parameter interaction: forcerelay implies relay
PF_FORCERELAY = (1U << 2) | PF_RELAY, PF_FORCERELAY = (1U << 2) | PF_RELAY,
// Can't be banned/disconnected/discouraged for misbehavior // Can't be banned/disconnected/discouraged for misbehavior

View File

@ -1167,17 +1167,6 @@ bool IsBanned(NodeId pnode)
return false; return false;
} }
/**
* 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 CValidationState& state)
{
assert(IsTransactionReason(state.GetReason()));
return state.GetReason() == ValidationInvalidReason::CONSENSUS;
}
/** /**
* Potentially mark a node discouraged based on the contents of a CValidationState object * Potentially mark a node discouraged based on the contents of a CValidationState object
* *
@ -1187,10 +1176,9 @@ static bool TxRelayMayResultInDisconnect(const CValidationState& state)
* txs, the peer should not be punished. See BIP 152. * txs, the peer should not be punished. See BIP 152.
* *
* @return Returns true if the peer was punished (probably disconnected) * @return Returns true if the peer was punished (probably disconnected)
*
* Changes here may need to be reflected in TxRelayMayResultInDisconnect().
*/ */
static bool MaybePunishNode(NodeId nodeid, const CValidationState& state, bool via_compact_block, const std::string& message = "") { static bool MaybePunishNode(NodeId nodeid, const CValidationState& state, bool via_compact_block, const std::string& message = "")
{
switch (state.GetReason()) { switch (state.GetReason()) {
case ValidationInvalidReason::NONE: case ValidationInvalidReason::NONE:
break; break;
@ -1254,12 +1242,6 @@ static bool MaybePunishNode(NodeId nodeid, const CValidationState& state, bool v
} }
////////////////////////////////////////////////////////////////////////////// //////////////////////////////////////////////////////////////////////////////
// //
// blockchain -> download logic notification // blockchain -> download logic notification
@ -3469,14 +3451,11 @@ bool ProcessMessage(CNode* pfrom, const std::string& msg_type, CDataStream& vRec
if (pfrom->HasPermission(PF_FORCERELAY)) { if (pfrom->HasPermission(PF_FORCERELAY)) {
// Always relay transactions received from whitelisted peers, even // Always relay transactions received from whitelisted peers, even
// if they were already in the mempool or rejected from it due // if they were already in the mempool,
// to policy, allowing the node to function as a gateway for // allowing the node to function as a gateway for
// nodes hidden behind it. // nodes hidden behind it.
// if (!mempool.exists(tx.GetHash())) {
// Never relay transactions that might result in being LogPrintf("Not relaying non-mempool transaction %s from whitelisted peer=%d\n", tx.GetHash().ToString(), pfrom->GetId());
// 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));
} else { } else {
LogPrintf("Force relaying tx %s from whitelisted peer=%d\n", tx.GetHash().ToString(), pfrom->GetId()); LogPrintf("Force relaying tx %s from whitelisted peer=%d\n", tx.GetHash().ToString(), pfrom->GetId());
RelayTransaction(tx.GetHash(), *connman); RelayTransaction(tx.GetHash(), *connman);

View File

@ -140,6 +140,16 @@ class P2PPermissionsTests(BitcoinTestFramework):
p2p_rebroadcast_wallet.send_txs_and_test([tx], self.nodes[1]) p2p_rebroadcast_wallet.send_txs_and_test([tx], self.nodes[1])
wait_until(in_mempool) wait_until(in_mempool)
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): def checkpermission(self, args, expectedPermissions, whitelisted):
self.restart_node(1, args) self.restart_node(1, args)
self.connect_nodes(0, 1) self.connect_nodes(0, 1)