net: Avoid SetTxRelay for feeler connections#26396
Hidden character warning
Conversation
mzumsande
left a comment
mzumsande
left a comment
There was a problem hiding this comment.
Concept ACK.
This has been suggested before (#22778 (comment))
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, 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. |
maflcko
force-pushed
the
2210-feeler-less-memory-
branch
from
faea347 to
fa24239
Compare
| peer.peer_disconnect() | ||
|
|
||
| self.log.info("SENDTXRCNCL should not be sent if feeler") | ||
| peer = self.nodes[0].add_outbound_p2p_connection(P2PFeelerReceiver(), p2p_idx=2, connection_type="feeler") |
There was a problem hiding this comment.
Shouldn't that be p2p_idx=3 and the below bumped to p2p_idx=4?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ok, then this is functionally fine. Could be somewhat confusing because surrounding tests do disconnect and bump p2p_idx after that.
There was a problem hiding this comment.
If changed, I think that this should be cleaned up to use an incremented variable like in p2p_add_connections.py to make future changes easier, but not necessary to do this here and invalidate ACKs. Maybe include in #26359 @naumenkogs?
There was a problem hiding this comment.
@mzumsande I think it makes sense to just set them all to 1, because it doesn't matter, rather than handle increments.
There was a problem hiding this comment.
@MarcoFalke @naumenkogs After looking at current CI failures such as https://cirrus-ci.com/task/5511952184115200?logs=ci#L4024, I think it does matter:
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.
There was a problem hiding this comment.
Good point! I think you're right, I've been looking at it for a bit and haven't found a better explanation. Please have this requirement documented in the PR you gonna open :)
|
Should we add |
|
Not sure. addrfetch may live up to 5 minutes and they are sent version.relay=True. So if this is changed, it should be done at the same time that changes relay=False. Maybe leave for a separate pull request, as this one is mostly about memory allocation? |
|
ACK fa24239 |
mzumsande
left a comment
mzumsande
left a comment
There was a problem hiding this comment.
ACK fa24239
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.
| peer.peer_disconnect() | ||
|
|
||
| self.log.info("SENDTXRCNCL should not be sent if feeler") | ||
| peer = self.nodes[0].add_outbound_p2p_connection(P2PFeelerReceiver(), p2p_idx=2, connection_type="feeler") |
There was a problem hiding this comment.
If changed, I think that this should be cleaned up to use an incremented variable like in p2p_add_connections.py to make future changes easier, but not necessary to do this here and invalidate ACKs. Maybe include in #26359 @naumenkogs?
|
@mzumsande my initial motivation was not even memory, but just for the code to be more logical :) |
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. |
6 participants
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_sendtxrcnclbefore disconnecting. This shouldn't matter, as other messages, such asmsg_wtxidrelay,msg_sendaddrv2,msg_verackormsg_getaddrare still sent. Though, it allows to test the changes here as a side-effect.