Seems odd to reserve memory for the struct (the heaviest member being m_tx_inventory_known_filter) when it is never used.
This also avoids sending out msg_sendtxrcncl before disconnecting. This shouldn’t matter, as other messages, such as msg_wtxidrelay, msg_sendaddrv2, msg_verack or msg_getaddr are still sent. Though, it allows to test the changes here as a side-effect.
488@@ -489,9 +489,8 @@ class CNode
489 /** Whether this peer provides all services that we want. Used for eviction decisions */
490 std::atomic_bool m_has_all_wanted_services{false};
491
492- /** Whether we should relay transactions to this peer (their version
493- * message did not include fRelay=false and this is not a block-relay-only
494- * connection). This only changes from false to true. It will never change
495+ /** Whether we should relay transactions to this peer.
496+ * This only changes from false to true. It will never change
497 * back to false. Used only in inbound eviction logic. */
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.
168@@ -163,6 +169,11 @@ def run_test(self):
169 assert not peer.sendtxrcncl_msg_received
170 peer.peer_disconnect()
171
172+ self.log.info("SENDTXRCNCL should not be sent if feeler")
173+ peer = self.nodes[0].add_outbound_p2p_connection(P2PFeelerReceiver(), p2p_idx=2, connection_type="feeler")
p2p_idx=3 and the below bumped to p2p_idx=4?
This is only used to pick a port (if none was given). The previous connection was closed, so the index can be reused.
Though, happy to change this test or remove it, since it isn’t too important.
p2p_add_connections.py to make future changes easier, but not necessary to do this here and invalidate ACKs. Maybe include in #26359 @naumenkogs?
peer_disconnect doesn’t wait until the node has completed the disconnection. So there is a race between setting up the next connection (next addconnection RPC), and if the old one hasn’t been removed and is identical to the new one (because we didn’t increment the ID), CConnman::OpenNetworkConnection just returns without establishing a connection.
AddrFetch to this?
ACK fa24239a1c2281f61ab70a62228e88f4c7e72701
Should we add
AddrFetchto this?
I agree that this shouldn’t be done here, but fwiw I’m hesitant about this in general. In the rare occasions where we do AddrFetch connections, we usually have an empty addrman, few (if any) other peers and are most vulnerable to eclipse attacks. It might be better not to implement behavior from which the peer can conclude already at version negotation that this is likely an AddrFetch connection instead of a normal outbound connection, if the benefit is just to save a little memory on a connection that is short-lived anyway.
I don’t care much, but what would be wrong if the peer new that this is AddrFetch?
I was thinking of an attacker that would specifically target new and vulnerable nodes in an eclipse attack, giving legitimate GETADDR answer to other peers, so it would be hard to detect what this attacker is doing. But maybe that is a bit too hypothetical.
