p2p: Add DISABLETX message for negotiating block-relay-only connections #20726
Conversation
sdaftuar
force-pushed
the
2020-12-negotiate-block-relay
branch
from
3350a39
to
47994f4
Compare
|
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. |
There was a problem hiding this comment.
Concept ACK, and the implementation seems reasonably simple.
If I understand it correctly, the BIP idea doesn't concern itself with this being outbound->inbound or not, but the implementation only ever announces it in the outbound direction, and will disconnect if it's received from a (full) outbound peer. Is that worth pointing out (e.g. "Peers MAY decide to not serve peers that announce this flag").
Should interaction with tx-relay specific other messages like mempool, filterload, filteradd, filterclear, feefilter be specified (e.g. these messages are disabled/forbidden/ignored on block only connections)?
Maybe it's worth pointing out that blocktxn/getblocktxn still function?
src/net_processing.cpp
Outdated
| pfrom.UpdateConnectionType(ConnectionType::INBOUND_BLOCK_RELAY); | ||
| } | ||
| // If an outbound peer (manual or block-relay-only) sends us this | ||
| // message, we just ignore it. We won't relay transactions with such |
There was a problem hiding this comment.
We do relay transactions with manual peers, right?
There was a problem hiding this comment.
I should update the comment to be clearer. What I meant to describe was that we only disconnect an outbound peer who sends us the BLOCKRELAY message if we expect them to be a tx-relay or addr-relay peer. If we are connecting to them as a MANUAL connection, then we'll stay connected and just not relay transactions on that link.
In addition to fixing the comment, I could improve the code to ensure we don't advertise addrs to such peers either -- I think that will be easy to do at the same time that I ensure that a BLOCKRELAY peer doesn't send us other disallowed messages (mempool, filterload/add/clear, etc).
There was a problem hiding this comment.
Would some users be using -addnode to start with a trusted peer that can then also provide addrs? If so, when we receive BLOCKRELAY from a MANUAL connection, and if addrman is empty, is it worth alerting the user by logging?
|
Concept ACK |
1 similar comment
|
Concept ACK |
|
Concept ACK to having a way to not send Did you consider doing this implicitly, i.e. only consider a peer an addr-relay peer if:
For inbound peers we would defer initializing the addr-relay data structures and considering the peer an addr-relay peer until we receive the first address traffic from it. I think this would achieve the goals you want (reduce bandwidth, avoid relaying addresses to black holes, potentially reduce memory usage in future) without the need for a protocol change. |
|
@sipa Thanks for taking a look!
Yes good observation. When I was working on this, I realized that there was no protocol need to restrict this as being a message from inbound->outbound peer, so while I didn't have an immediate use case for outbound peers sending this message to inbound ones, I also thought there seemed to be little downside to permitting it in the future. The only case where I thought it could be problematic is if we connect out to a peer that we expect to be a full-relay peer, and it turns out the peer tells us we won't get any transactions from them -- in this case I think we ought to disconnect. (Arguably it's a bug that today we don't do this when we get fRelay=false from such a peer in their version message.) I'll update the BIP to reflect that peers should feel free to use this information in deciding who to stay connected to.
Yes, I should both specify that those messages are not permitted, and I think disconnect peers who send them to us after negotiating BLOCKRELAY. I think this will require an extra piece of state for outbound peers who might send us a BLOCKRELAY message, but that seems no big deal.
I'll update the BIP to explicitly reflect this as well. |
@jnewbery I think we could improve addr-relay by making guesses about our peers, but it seems to me that the logic is much clearer if we just explicitly add support for peers telling each other what they're trying to do. Updating the protocol with a new message type is not particularly difficult or costly; having implementations guess at what a peer is doing based on observed behavior seems much more error-prone and increases the maintenance burden on the project with additional code complexity. I'd go further and suggest that in the future, we consider adding some kind of p2p message to explicitly negotiate addr-relay behavior (perhaps by indicating for which networks a peer is interested in gossiping addresses).
I don't think this would achieve a nice way to eliminate the |
|
I'm not suggesting that we make guesses or have workarounds. The proposal is that peers opt in to address relay by relaying addresses. That's the most reliable heuristic we have, and it works reliably for all peers on the network, not just those that have upgraded to version 70018. This would work today with clients that connect to us and that are addr relay black holes.
I've implemented my suggestion here: https://github.com/bitcoin/bitcoin/compare/master...jnewbery:2020-12-implicit-addr-relay?expand=1. It's a 20 line change, and doesn't seem too complex.
I didn't implement this on my branch, but it'd be easy to do. Just defer initializing the m_tx_relay pointer until the peer has implicitly opted in to tx relay (but that seems to be a different issue from what you're trying to accomplish in this PR). One problem with a new |
The downside to an approach like this (for both For some additional context, please see #15759 (comment) and #19858 (comment). Note also the ambiguity of our current approach to guessing that a peer is an inbound-block-relay-only peer, such as was done in #19670. |
|
@Limpisey168 If you're going to review, please leave meaningful comments. |
src/net.cpp
Outdated
| assert(new_conn_type == ConnectionType::INBOUND_BLOCK_RELAY); | ||
| m_conn_type = new_conn_type; | ||
| // We can now drop some unnecessary data structures. | ||
| // TODO: deallocate m_tx_relay and m_addr_known. |
There was a problem hiding this comment.
Does it make sense to have m_conn_type be std::atomic (rather than protected by a mutex) if updating it is going to also invalidate other members?
It's not clear to me that having m_conn_type be modified is actually the right approach -- we don't change the connection type when a peer wants us to send compact block announcements or not, eg. It might make more sense to have the connection type stay as "inbound" but have an extra flag for "want blocks-only behaviour" -- that flag could just be m_tx_relay == nullptr at which point you don't need to have locking to keep m_tx_relay and m_conn_type in sync. You still need something to ensure you don't free m_tx_relay while someone else might be using it, but that could be just changing it to a shared_ptr?
Actually, perhaps m_tx_relay simply shouldn't be allocated until you've received their VERACK, rather than contemplating deallocating it?
There was a problem hiding this comment.
I am confused by what constitutes a new connection type v/s a negotiated feature of a connection. Other negotiated features that do not seem to change the connection type include WTXIDRELAY,SENDADDRV2, SENDCMPCT, etc.
As a starting point, I'd suggest that connection types imply (1) a different level of trust in the peer (eg. INBOUND vs OUTBOUND) or (2) connection limits(eg. 2 for OUTBOUND_BLOCK_RELAY, 1 for FEELER) and all other features are attributes of a connection. This also opens up future possibilities for peers that want other combinations like block and addr relay, but no tx relay, etc.
There was a problem hiding this comment.
I wondered whether the better approach would be to add a new bool to indicate whether tx-relay is disabled (like we have for compact blocks, or sendheaders, etc), or to update the connection type like I've done here.
My hesitation to add a new bool was that it seemed counter to the recent work to refactor net to use connection types as an alternative to the bucket-of-bool's approach for deciding how to interact with a peer. If we have a set of semantics that we understand go together for a given peer, it's nice for code readability to encode that as simply as possible.
On the other hand, my intention with the BIP was to allow future flexibility for what semantics might be possible (eg my intention is to allow for addr-relay to happen in the future if we add an explicit way to negotiate addr relay, as an example), so from that perspective, having a connection type perhaps implies too much of an understanding of what the peer may be doing. And for inbound peers in particular, we never really know what they're doing, so perhaps having more than just "inbound" as a connection type may be misleading to future code readers as well.
I like the idea of moving initialization of m_tx_relay to VERACK, but not sure if I'll end up tackling that here or deferring for the current refactoring work in progress to make more headway (I think we need to add a lock that guards access to the m_tx_relay pointer itself?)
There was a problem hiding this comment.
I think we've got a few different ways in which nodes differ:
- which random features they support (wtxid relay, compact blocks, addrv2; erlay, pkg relay, p2p encryption in the future) that might make things more efficient (and which we might prefer our connections to support) but don't really change anything
- how much we trust them (manually privileged/selected, we chose them as an outbound, they chose us as an inbound)
- why we've got the connection (manual setup for some higher level purpose, tx relay, block relay, feeler, addrfetch)
- which limits we apply to them (8 outbounds, 2 outbound block relay, 115 (?) inbounds)
We should use bools instead of conn_type for random features (otherwise we either get an exponential explosion of types or have to sequence features), and I don't think trust/"why" really affects inbounds that only wants blocks or not, but it could make sense to have a new limit imply a new connection-type? Given the limits overlap a little (extra outbounds vs feelers), and what to do when you have extra peers requires looking at other data anyway, I'm not sure that's very convincing though.
Hmm, how much we "trust" a connection might change over the course of a connection in future if we allow random inbound peers to authenticate themselves to us... So I guess that leaves me thinking that we should have const m_conn_type representing our purpose in having the connection? (Different purposes meaning substantial changes in the protocol state machine -- eg, blocks only we deliberately opt out of tx and addr relay even if we otherwise support both, feelers we disconnect after verack, addrfetch we disconnect after getting addresses, inbounds we have different delays for and send VERSION later)
(I think we need to add a lock that guards access to the m_tx_relay pointer itself?)
Maybe? I think there are already way more per-peer mutexes than are really useful...
There was a problem hiding this comment.
strong concept ACK - having connections explicitly declare expectations around what-to-relay makes a lot of sense. both for the current state as well as the future direction you've indicated.
that said, I have lots of thoughts to consider around approach :)
BIP:
my main questions are around other use cases & long term plans. the current proposal involves sending a singular message that disables 2/3 inventory types, but what about other circumstances where a peer might not want to participate in addr or tx relay?
as a current example, it would make sense for a node that has enabled -connect to not participate in addr relay for all the same reasons as this PR: it would blackhole any self-announcements and its unnecessary bandwidth & memory usage.
based on conversations that arose when you originally implemented block-relay-only connections, I could imagine a future where we implement connections that permit block & addr relay, but disable tx relay.
I'm not suggesting we need to implement all of this right away, but I'd like to understand the high level direction so we can to leave the door open for future improvements, since this BIP will set a precedence.
one route that makes sense to me- a message format that enables peers to signal which inventory types they would like to subscribe to (or unsubscribe from). either as separate messages, or a single message with multiple fields. if we go an opt-in route, we could also defer initializing the relevant data structures (m_tx_relay & m_addr_known) until we receive the message indicating the peer wants to relay that type of message.
I think this might be similar to ideas that john & aj have touched on in their reviews.
Implementation: Approach
- I think I agree with aj's p2p: Add DISABLETX message for negotiating block-relay-only connections #20726 (comment) that
INBOUND_BLOCK_RELAYmight be better represented as theINBOUNDconnection type with the additional state maintaining if tx/addr relay is enabled. I believe it would reduce the diff, and in the places where we want to discern the two, the additional check of data structures is useful / relevant anyways.
I was looking into the m_tx_relay and m_addr_known data structures, trying to come up with ideas for improving the code paths. (I introduced some of the current state, but its not great.) I also saw your proposal and the review conversation in #20676. I don't have any amazing solutions, but I have one or two thoughts to consider:
- whatever route we go, would be nice if
m_tx_relayandm_addr_knownare handled similarly. right now they are a bit out of sync (eg. the check for instantiating in the constructor) - one option:
CNodeconstructor checks connection types to decide whether or not to instantiate the object, and theRelay[xxxx]WithConn()function can check for presence of the object. I think that would mean the connection type -> data structure relationship is only defined once, the structures could be deallocated without problems, and we have code safety of not dereferencing null pointers without needing to blur the layers? - as I already mentioned, I like the idea around deferring the creation of these structures.
Implementation: Specifics
(sorry if its too early for some of these comments)
- we'll definitely need to fix the remote crash bug :)
- if we do keep
INBOUND_BLOCK_RELAYas a separate connection type, might be worth adding to the check here: https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L2200 - the
IsBlockOnlyConnfunction is now extra confusing. It should either be updated to return both types, or the name changed (probably the latter).
Questions
- Protect localhost and block-relay-only peers from eviction #19670 introduced a workaround to deduce inbound block-relay-only peers and protect them from eviction. After this PR, do you think we could revisit to make it more direct?
There was a problem hiding this comment.
Overall I agree on PR/BIP aims to reduce per-peer memory for node and peer upstream bandwidth dedicated to useless tx/addr relay.
I still need to think about this new connection negotiation mechanism where a traffic class opt-in (blocks) implies opt-out from all other traffic classes (addr, tx). Maybe the BIP should have a section explaining that block-relay is the primary traffic class and that a peer can only escalate from there (e.g to addr-relay by sending after a ADDRRELAY).
But this direction may have issues. E.g if a future p2p extension governing addr-relay is neutral for block-relay, how would you enable persistent addr-relay-only connections ?
src/net.cpp
Outdated
| assert(new_conn_type == ConnectionType::INBOUND_BLOCK_RELAY); | ||
| m_conn_type = new_conn_type; | ||
| // We can now drop some unnecessary data structures. | ||
| // TODO: deallocate m_tx_relay and m_addr_known. |
There was a problem hiding this comment.
I am confused by what constitutes a new connection type v/s a negotiated feature of a connection. Other negotiated features that do not seem to change the connection type include WTXIDRELAY,SENDADDRV2, SENDCMPCT, etc.
As a starting point, I'd suggest that connection types imply (1) a different level of trust in the peer (eg. INBOUND vs OUTBOUND) or (2) connection limits(eg. 2 for OUTBOUND_BLOCK_RELAY, 1 for FEELER) and all other features are attributes of a connection. This also opens up future possibilities for peers that want other combinations like block and addr relay, but no tx relay, etc.
src/net_processing.cpp
Outdated
| pfrom.UpdateConnectionType(ConnectionType::INBOUND_BLOCK_RELAY); | ||
| } | ||
| // If an outbound peer (manual or block-relay-only) sends us this | ||
| // message, we just ignore it. We won't relay transactions with such |
There was a problem hiding this comment.
Would some users be using -addnode to start with a trusted peer that can then also provide addrs? If so, when we receive BLOCKRELAY from a MANUAL connection, and if addrman is empty, is it worth alerting the user by logging?
There was a problem hiding this comment.
Should interaction with tx-relay specific other messages like mempool, filterload, filteradd, filterclear, feefilter be specified (e.g. these messages are disabled/forbidden/ignored on block only connections)?
Yes, I should both specify that those messages are not permitted, and I think disconnect peers who send them to us after negotiating BLOCKRELAY. I think this will require an extra piece of state for outbound peers who might send us a BLOCKRELAY message, but that seems no big deal.
With this implementation we'll continue to accept feefilter messages from, and send feefilter messages to, peers which have send us disabletx. Is that desired/intended? Should the behaviour be specified in the BIP?
This seems to be an oversight -- will update the BIP/code to restrict these messages too. |
Outbound full-relay peers that send us this get disconnected. Additionally disconnect if a disabletx peer sends: - any bip37 message (filterload/filteradd/filterclear) - feefilter - mempool message - getdata(tx) - getdata(merkleblock) - notfound(tx) - inv(tx)
Verify that bitcoind sends it to block-relay-only peers, and that bitcoind will disconnect peers that send disabletx but are not in compliance with BIP 338.
sdaftuar
force-pushed
the
2020-12-negotiate-block-relay
branch
from
41e8257
to
791048b
Compare
|
Updated this PR to restrict feefilter messages from disabletx peers. See also bitcoin/bips#1287. @jnewbery I didn't explicitly remove the sending of FEEFILTER messages to peers that have sent us a DISABLETX -- that behavior will automatically change, based on the current logic, once we stop instantiating a |
There was a problem hiding this comment.
I didn't explicitly remove the sending of FEEFILTER messages to peers that have sent us a DISABLETX -- that behavior will automatically change, based on the current logic, once we stop instantiating a m_tx_relay object for the peer
I'm confused. That is the case for the local node selecting the peer as an outbound block-relay-only. The Peer's m_tx_relay is set to false in InitializeNode and no TxRelay object is instantiated. However, on the other side of the connection, the local node is seen as is IsInboundConn() and a TxRelay object allocated, allowing the send of FEEFILTER in MaybeSendFeefilter. So an outbound block-relay-only might send FEEFILTER messages provoking a disconnection with current code ? Or are you referencing another behavor or am I missing an interference with fRelay ?
| @@ -4085,6 +4138,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, | |||
| } | |||
|
|
|||
| if (msg_type == NetMsgType::FEEFILTER) { | |||
| if (peer->m_disable_tx) { | |||
There was a problem hiding this comment.
If you have to retouch the code, you can increase the clarity of the logs by precising which protocol violation has been hit, in the occurrence a forbidden sending after a "disable tx received" (as some logs are actually precising). Nice for the node operator.
I think the idea is just that after |
I was looking at this in the context of #22778. It seems to me that |
Otherwise, I think renaming the |
Yes, I'm aligned on the direction to remove |
Here's how I think it works:
|
|
Hmm, having the behaviour be "must not send message X if you receive the DISABLETX flag" seems a bit incompatible with upgrades? Suppose protocol version 70020 comes along with erlay support, and you (a non-bitcoin-core implemention) implement that, but don't bother with disabletx. Then a block_relay_only connection comes in, and says "oh, high version number, I'll send the DISABLETX msg", which you then ignore. You don't send txs to them because they specified Wouldn't it be better to treat feature support and enablement separately; ie send "disabletx false" normally, and "disabletx true" for block-relay-only outbounds, with the logic being "if both side support disabletx, and either side enables it, then don't send tx related messages and disconnect if they're received; otherwise no-op" ? |
@ajtowns I don't think the code suffers from the problem that you describe, because this BIP and implementation do not enforce any restrictions on recipients of a I'll rework the BIP and this code to implement that, thank you! |
Thanks, I understand my confusion. The |
|
Updated bip text is here: sdaftuar/bips@ecf86e1. Will update this branch next. |
|
🐙 This pull request conflicts with the target branch and needs rebase. Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft". |
|
Closing now that #22778 has been merged. |
Implement BIP 338.
When we initiate a block-relay-only connection today, our peer doesn't know that we won't send transactions ourselves, or even that we won't try to turn on transaction relay at some later point during the connection's lifetime.
This PR adds a new p2p message, DISABLETX, to be sent between VERSION and VERACK so that peers can signal to each other that they only want blocks/compactblocks/headers to be sent on the link, and not transaction-relay traffic (or addr messages, unless otherwise indicated).