p2p: Add DISABLETX message for negotiating block-relay-only connections #20726

sdaftuar commented at 8:55 PM on December 19, 2020: member

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).

DrahtBot added the label P2P on Dec 19, 2020

DrahtBot added the label RPC/REST/ZMQ on Dec 19, 2020

sdaftuar force-pushed on Dec 19, 2020

DrahtBot commented at 12:26 AM on December 20, 2020: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25156 (refactor: Introduce PeerManagerImpl::RejectIncomingTxs by MarcoFalke)
#24970 (net processing: Move cleanSubVer, fPreferredDownload and nLocalHostNonce to Peer by jnewbery)
#24128 (wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs by MarcoFalke)
#23443 (p2p: Erlay support signaling by naumenkogs)
#22778 (net processing: Reduce resource usage for inbound block-relay-only connections by jnewbery)
#21515 (Erlay: bandwidth-efficient transaction relay protocol by naumenkogs)

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.

in src/net_processing.cpp:2597 in 47994f4ec9 outdated

2592 | +        }
2593 | +        if (pfrom.IsInboundConn()) {
2594 | +            pfrom.UpdateConnectionType(ConnectionType::INBOUND_BLOCK_RELAY);
2595 | +        }
2596 | +        // If an outbound peer (manual or block-relay-only) sends us this
2597 | +        // message, we just ignore it. We won't relay transactions with such

sipa commented at 5:21 AM on December 20, 2020:

We do relay transactions with manual peers, right?

sdaftuar commented at 4:56 PM on December 20, 2020:

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).

dhruv commented at 6:22 PM on December 28, 2020:

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?

sipa commented at 5:29 AM on December 20, 2020: member

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?

jonatack commented at 8:27 AM on December 20, 2020: member

Concept ACK

laanwj commented at 12:02 PM on December 20, 2020: member

Concept ACK

jnewbery commented at 12:32 PM on December 20, 2020: member

Concept ACK to having a way to not send addr messages to inbound block-relay-only peers.

Did you consider doing this implicitly, i.e. only consider a peer an addr-relay peer if:

it's an outbound peer that isn't block-relay-peer; or
it's an inbound peer and we've received an addr, addrv2 or getaddr from the peer.

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.

sdaftuar commented at 4:52 PM on December 20, 2020: member

@sipa Thanks for taking a look!

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").

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.

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.

Maybe it's worth pointing out that blocktxn/getblocktxn still function?

I'll update the BIP to explicitly reflect this as well.

sdaftuar commented at 3:55 AM on December 21, 2020: member

Did you consider doing this implicitly, i.e. only consider a peer an addr-relay peer if: @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).

it's an outbound peer that isn't block-relay-peer; or

it's an inbound peer and we've received an addr, addrv2 or getaddr from the peer.

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.

I don't think this would achieve a nice way to eliminate the m_tx_relay pointer that we keep per-peer, because the existing protocol doesn't have a good way to communicate that transaction relay will never be enabled on the link. At any rate, finding a workaround to try to infer the desired behavior seems unnecessarily complex -- better to document it in a BIP and add a simple way to understand what is happening.

jnewbery commented at 9:39 AM on December 21, 2020: member

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.

... increases the maintenance burden on the project with additional code complexity.

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 don't think this would achieve a nice way to eliminate the m_tx_relay pointer that we keep per-peer, because the existing protocol doesn't have a good way to communicate that transaction relay will never be enabled on the link.

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 blockrelay message is that it's peculiarly specific to the Bitcoin Core implementation at this point in time. It's the first feature negotiation message that opts out of certain features (tx, inv, getdata(tx)) but remains silent on other features or potential future features (are cfilters supported on inbound-block-relay peers? What about Erlay/sendrecon/etc?)

sdaftuar commented at 5:25 PM on December 21, 2020: member

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.

The downside to an approach like this (for both m_addr_known and m_tx_relay) is that we don't get the benefit of limiting and bounding the resources that will be used on the peer at the time of connection. Adding a p2p message that is sent at the start of a connection (such as the one proposed here) allows us to do that. Once we have a way to bound the resources of a peer that is connecting to us with this intent, I plan to propose that we increase the number of inbound connection slots that we can service, and reserve some of those for BLOCKRELAY peers. From there, I think we could then start to increase the number of outbound block-relay-only connections we make without concern that we are taking over too many inbound connection slots on the network or using up too many node resources.

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.

jnewbery commented at 1:34 PM on December 22, 2020: member

@sdaftuar Thanks for the additional context. This makes much more sense to me now that I understand the longer term plan. The change in #19670 to favour inbound-block-relay-only peers also makes much more sense when we're limiting the memory usage of inbound-block-relay-only peers.

Concept ACK.

Limpisey168 approved

sipa commented at 7:00 PM on December 23, 2020: member

@Limpisey168 If you're going to review, please leave meaningful comments.

in src/net.cpp:569 in 6f0a312ebc outdated

 564 | +{
 565 | +    assert(m_conn_type == ConnectionType::INBOUND);
 566 | +    assert(new_conn_type == ConnectionType::INBOUND_BLOCK_RELAY);
 567 | +    m_conn_type = new_conn_type;
 568 | +    // We can now drop some unnecessary data structures.
 569 | +    // TODO: deallocate m_tx_relay and m_addr_known.

ajtowns commented at 1:51 AM on December 24, 2020:

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?

dhruv commented at 6:12 PM on December 28, 2020:

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.

sdaftuar commented at 7:23 PM on December 28, 2020:

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?)

ajtowns commented at 4:42 AM on December 30, 2020:

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...

in src/net_processing.cpp:2575 in 2668c8e239 outdated

2571 | @@ -2566,6 +2572,18 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
2572 |          return;
2573 |      }
2574 |  
2575 | +    if (msg_type == NetMsgType::BLOCKRELAY) {

ajtowns commented at 2:20 AM on December 24, 2020:

Wouldn't "BLKRELAYONLY" be a better name for this concept, or even separating it out into its components as "TXRELAY false" and "ADDRRELAY false" ? Seems surprising that a "BLOCKRELAY" message would change tx and addr relay behaviour, but not change block relay behaviour...

sdaftuar commented at 7:23 PM on December 28, 2020:

How about disabletx?

ajtowns commented at 4:46 AM on December 30, 2020:

Sure, disabletx or txrelay false sound about the same to me; though opting-out vs opting-in still maybe seems odd?

sdaftuar commented at 1:53 PM on January 5, 2021:

Well my thinking is that the default on the network is that everything gets relayed, so the natural thing to add is an opt-out.

ajtowns commented at 2:40 AM on December 24, 2020: member

Concept ACK

in src/net_processing.cpp:2593 in 47994f4ec9 outdated

2588 | +        if (pfrom.m_tx_relay != nullptr && WITH_LOCK(pfrom.m_tx_relay->cs_filter, return pfrom.m_tx_relay->fRelayTxes)) {
2589 | +            // Can't send a BLOCKRELAY message if they didn't turn off tx-relay
2590 | +            // in the version message.
2591 | +            pfrom.fDisconnect = true;
2592 | +        }
2593 | +        if (pfrom.IsInboundConn()) {

amitiuttarwar commented at 2:57 AM on December 24, 2020:

I thiiink this could be problematic if the peer sends two BLOCKRELAY messages in a row?

initially ConnectionType::INBOUND
they send first BLOCKRELAY message, we hit this code & update to ConnectionType::INBOUND_BLOCK_RELAY
they send another BLOCKRELAY message, we hit this code again because IsInboundConn() would be true
UpdateConnectionType is invoked, which asserts the connection type is ConnectionType::INBOUND.

ariard commented at 12:24 AM on December 28, 2020:

Nice found!

If the code purpose of the sequence of assert was to validate the ConnectionType state transition before to operate it, another approach could be to resort to a switch statement, far less harmful and easier to reason on ?

diff --git a/src/net.cpp b/src/net.cpp
index 8027723b5..086fbf227 100644
--- a/src/net.cpp
+++ b/src/net.cpp
@@ -562,11 +562,17 @@ Network CNode::ConnectedThroughNetwork() const
 
 void CNode::UpdateConnectionType(ConnectionType new_conn_type)
 {
-    assert(m_conn_type == ConnectionType::INBOUND);
-    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.
+    switch (m_conn_type) {
+        case ConnectionType::INBOUND:
+            if (new_conn_type == ConnectionType::INBOUND_BLOCK_RELAY) m_conn_type = new_conn_type;
+        case ConnectionType::INBOUND_BLOCK_RELAY:
+        case ConnectionType::MANUAL:
+        case ConnectionType::OUTBOUND_FULL_RELAY:
+        case ConnectionType::BLOCK_RELAY:
+        case ConnectionType::FEELER:
+        case ConnectionType::ADDR_FETCH:
+            break;
+    }
 }
 
 #undef X

in src/version.h:12 in 47994f4ec9 outdated

   8 | @@ -9,7 +9,7 @@
   9 |   * network protocol versioning
  10 |   */
  11 |  
  12 | -static const int PROTOCOL_VERSION = 70016;
  13 | +static const int PROTOCOL_VERSION = 70018;

amitiuttarwar commented at 1:33 AM on December 25, 2020:

why does this protocol version number get bumped by 2?

sdaftuar commented at 7:24 PM on December 28, 2020:

I thought Erlay might be using one!

amitiuttarwar commented at 2:38 AM on December 25, 2020: contributor

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 #20726 (review) that INBOUND_BLOCK_RELAY might be better represented as the INBOUND connection 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_relay and m_addr_known are handled similarly. right now they are a bit out of sync (eg. the check for instantiating in the constructor)
one option: CNode constructor checks connection types to decide whether or not to instantiate the object, and the Relay[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_RELAY as a separate connection type, might be worth adding to the check here: https://github.com/bitcoin/bitcoin/blob/master/src/net.cpp#L2200
the IsBlockOnlyConn function is now extra confusing. It should either be updated to return both types, or the name changed (probably the latter).

Questions

#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?

amitiuttarwar removed the label RPC/REST/ZMQ on Dec 25, 2020

in src/net_processing.cpp:3525 in 6f0a312ebc outdated

3521 | @@ -3522,7 +3522,7 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
3522 |          // to users' AddrMan and later request them by sending getaddr messages.
3523 |          // Making nodes which are behind NAT and can only make outgoing connections ignore
3524 |          // the getaddr message mitigates the attack.
3525 | -        if (!pfrom.IsInboundConn()) {
3526 | +        if (!pfrom.IsInboundConn() || !pfrom.RelayAddrsWithConn()) {

ariard commented at 12:40 AM on December 28, 2020:

IIUC, after this commit, outbound block-relay-only peers are excluded from requesting addrs. Was this intended ? Further, the new connection type should already be excluded as IsInboundConn is extended in same commit.

dhruv commented at 6:00 PM on December 28, 2020:

@ariard If I am reading this correctly, outbound-block-relay peers would not get a response to getaddr even prior to this commit.

I am not sure I understand how the new connection type should already be excluded as:

!pfrom.IsInboundConn() => ( !INBOUND && !INBOUND_BLOCK_RELAY )
( !pfrom.IsInboundConn() || !pfrom.RelayAddrsWithConn() ) => ( !INBOUND || BLOCK_RELAY || INBOUND_BLOCK_RELAY )

ariard commented at 10:39 PM on December 29, 2020:

You're right that's an OR! Maybe we can rename RelayAddrsWithConn() to IsFullConn to reduce confusion, like other predicates on connection types. We're stretching slowly from the #19316 original goal of simplifying handling of connection types...

jonatack commented at 10:50 PM on December 29, 2020:

@ariard Agreed; this is the motivation for #20729. Feel free to suggest naming improvements to the scripted diff there.

in src/net_processing.cpp:2583 in d99991704c outdated

2578 | @@ -2579,8 +2579,23 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
2579 |              pfrom.fDisconnect = true;
2580 |              return;
2581 |          }
2582 | -        // ignore this for now - later we can downgrade the resources allocated
2583 | -        // to this peer.
2584 | +        if (pfrom.IsFullOutboundConn() || pfrom.IsAddrFetchConn()) {
2585 | +            // If we picked an outbound for tx- or addr-relay and it sends us a

ariard commented at 1:01 AM on December 28, 2020:

I wonder if the rational shouldn't be different. AFAICT, BLOCKRELAY aims to inform the connected node about the initiator expectations on traffic relayed on this link. So a connected node inverting the logic and announcing its link expectations will break the effect and force the initiator to adapt its peer resource allocation.

I would say the restriction here is motivated only by the outboundness of the connection, the traffic class (tx, addr, block-relay) shouldn't matter.

I think this PR and BIP introduce a "one-way" negotiation privilege for the initiator, which makes sense, but should this be documented in the BIP or stay an implicit assumption of our peer management policy ?

in src/net_processing.cpp:2821 in d99991704c outdated

2587 | +            pfrom.fDisconnect = true;
2588 | +            return;
2589 | +        }
2590 | +        if (pfrom.m_tx_relay != nullptr && WITH_LOCK(pfrom.m_tx_relay->cs_filter, return pfrom.m_tx_relay->fRelayTxes)) {
2591 | +            // Can't send a BLOCKRELAY message if they didn't turn off tx-relay
2592 | +            // in the version message.

ariard commented at 1:17 AM on December 28, 2020:

This is where I don't understand BIP following excerpt :

this is because the transaction relay field in the version message is not a permanent setting for the lifetime of the connection. Consequently, a node receiving an inbound connection with transaction relay disabled cannot distinguish between a peer that will never enable transaction relay and one that will.

Why do you mean exactly by "is not a permanent setting" ? Version message can only be received once (L2270). Further a peer which turns off tx-relay but then starts to relay tx would break its connection negotiation commitment and as such should be disconnected ? I don't think we do this for now.

And even further when we pick up a peer as a block-relay-only one, we have no word to say in its choice of turning off tx-relay field or not ?

sdaftuar commented at 1:59 PM on January 12, 2021:

Sending a FILTERCLEAR (at least, to a node with NODE_BLOOM set) will enable transaction relay; see BIP37.

ariard commented at 1:36 AM on December 28, 2020: member

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 ?

in src/net_processing.cpp:2957 in 2668c8e239 outdated

2571 | @@ -2566,6 +2572,18 @@ void PeerManager::ProcessMessage(CNode& pfrom, const std::string& msg_type, CDat
2572 |          return;
2573 |      }
2574 |  
2575 | +    if (msg_type == NetMsgType::BLOCKRELAY) {
2576 | +        if (pfrom.fSuccessfullyConnected) {

dhruv commented at 6:18 PM on December 28, 2020:

Out of curiosity, why must BLOCKRELAY be negotiated between VERSION and VERACK? The only reason I can think of is to avoid allocating (m_tx_relay, m_addr_known) altogether instead of allocating, then de-allocating. Is that correct?

sdaftuar commented at 6:46 PM on March 9, 2021:

My goal is for software to know at the time of connection what kind of peer it's got, so that we can do inbound peer accounting (and reserve spots for lightweight peers) more easily.

We wait for VERACK before we start communicating with a peer, so we should negotiate this behavior before that VERACK is sent.

dhruv commented at 6:42 PM on December 28, 2020: member

Concept ACK: BLOCK_RELAY connections are one of our strongest defenses against network partitioning attacks. Reducing the resource requirements for these connections will enable us to have more such connections, further increasing the cost of attacks.

luke-jr commented at 1:02 AM on January 3, 2021: member

a block-relay-only connection today, our peer doesn't know that we don't want addr relay on the link

These seem like they're conceptually different topics, and treating them as the same seems like an implementation-specific thing...

only want blocks/compactblocks/headers to be sent on the link, and not transaction-relay traffic

What is the purpose of compactblocks without transaction-relay traffic?

In the future, we can also save a bit of memory by not allocating (or deallocating) the peer data structures we maintain for transaction and address relay.

Can't we do that today, just by not allocating it until we know the peer wants it?

jnewbery commented at 10:56 AM on January 3, 2021: member

I agree with some of the other comments here that it seems too implementation-specific to encode "relay blocks, don't relay txs, don't relay addrs" in a single P2P message.

I think a more general approach would be to introduce three new P2P messages for relay negotiation:

sendblocks to opt in to block relay (can be followed by sendheaders and sendcmpct for those features)
sendtxs to opt in to tx relay (can be followed by wtxidrelay for that feature)
sendaddrs to opt in to addr relay (can be followed by sendaddrv2 for that feature)

(we could argue about whether those new send* messages should include message bodies that communicate which sub-features to opt into, but that detail isn't important for this discussion)

If the peer's version is >= 70017, the peer must send those messages during the negotiation phase to opt in to those inventory relays.

That approach seems much more general and is closer to the existing feature negotiation mechanism we use (where all features are opt-in). It also avoids any confusion about how this mechanism interacts with possible future features.

sdaftuar commented at 3:04 PM on January 5, 2021: member

Thanks all for the comments so far. I've renamed the p2p message to "disabletx" to more clearly communicate what the effect of processing this message is. Current draft is here: https://github.com/sdaftuar/bips/blob/2020-09-negotiate-block-relay/bip-disable-tx.mediawiki

Some highlights, besides the message being renamed:

I've made it more explicit that the BIP37-related messages are disallowed
I've clarified that addr messages should be avoided for now, but could be sent/received if in the future we deploy a way to negotiate addr relay explicitly. This accommodates existing software behavior without preventing us from cleanly separating these two ideas in the future.

I don't think it makes sense to add a "sendblocks" p2p message, because there is no use case I'm aware of for connections on the p2p network to not want blocks (really, headers or block hashes, because unrequested blocks are themselves not widely supported on the network). If there were such a use case, someone should feel free to propose this. For now, I think it's sufficient for this use case -- of negotiating block-relay-only connections -- to disable transaction relay, and listening nodes can use the disabling of transaction relay as proposed here as a proxy for "block-relay-only" connections, to facilitate increased connection limits in the future.

I do think that adding a way to opt-in to addr relay is desirable, however I think more research is required to figure out what that should look like. Naively, I'd think it makes sense to opt-in to addr-relay on a per-network-type basis (as described in BIP 155), but how exactly that should work is intertwined with what relay strategies might make sense, and I would not feel comfortable proposing anything at this time (for instance, I'd say that we should clarify what the goals of address relay are, analyze what kinds of relay strategies optimize for that goal along with any DoS tradeoffs that might be introduced, and then determine what our own implementation might look like, before we finalize a specification). If someone wants to work on such a proposal, I would encourage it. However the current BIP was drafted with the goal of being compatible with any future work in this area so I don't think this should be a blocker.

I'll wait a bit for more comments here, but my plan is to share this with the bitcoin-dev list soon and then return to the implementation questions that have come up so far.

sdaftuar commented at 6:04 PM on January 5, 2021: member

@amitiuttarwar Hopefully I addressed most of your comments/questions on the BIP and future direction I have in mind. Just wanted to answer this:

#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?

I think we would wait until this has been deployed for a while and then (potentially) change the eviction logic. Two things occur to me on this point:

Older software will continue to use the old way of (not) communicating their intentions, via the fRelayTxes flag. So to accommodate older software we may not want to switch to keying off this p2p message for a little while at least.
It seems most of the feedback has been to shy away from encoding overly broad semantics into a single p2p message; while I think that is fine it also means that from our peer's point of view, they will have to infer the connection "type" from the properties of the peer, rather than from the peer declaring its full intentions in a single message. I think for now we can treat "will never relay txes" as essentially synonymous with these block-relay connections, but perhaps in the future there will be some distinction, and all a peer can do is rely on some set of flags that it might use to preference some peers over others -- which might put us exactly back to where we are today, with us just keying off of whether fRelayTxes is false, anyway.

sdaftuar commented at 6:28 PM on January 5, 2021: member

a block-relay-only connection today, our peer doesn't know that we don't want addr relay on the link

These seem like they're conceptually different topics, and treating them as the same seems like an implementation-specific thing...

I agree with some of the other comments here that it seems too implementation-specific to encode "relay blocks, don't relay txs, don't relay addrs" in a single P2P message. @jnewbery @luke-jr I'm a bit confused about both of your critiques of this as too "implementation-specific". The goal of the this BIP process is to standardize/formalize these block-relay-only connections in a useful way -- in my view, this behavior has been a good thing to introduce and we want to make more of these connections and so it is worth standardizing.

So I think the question is what is the best way to include this in the protocol. I think there are some good reasons to make this narrowly tailored to the exact thing we need -- disabling of transaction relay -- but we should be mindful that comes with some cost to how these connections can be understood by the recipient (now and going forward as the p2p protocol evolves), and therefore code complexity in the implementation. At any rate, I think I'm happy with the latest draft that spells out clearly that transaction relay must be disabled, and address relay is discouraged in the absence of any explicit negotiation for it (leaving the door open for a future addr relay protocol to govern addr behavior).

What is the purpose of compactblocks without transaction-relay traffic? @luke-jr You asked this question back when block-relay-only connections were introduced (https://github.com/bitcoin/bitcoin/pull/15759#issuecomment-484634796), and I answered this question back then as well (https://github.com/bitcoin/bitcoin/pull/15759#issuecomment-484666402). Nodes using these connections are welcome to also have full-relay connections to other peers, and therefore we might expect compact block relay to work just fine over block-relay-only links. I would recommend looking at the logs of your own node(s) to see how well compact block relay works on the existing block-relay-only peers that we have; in my experience it works quite well with some block-relay-only peers being selected as HB peers (and vice versa).

From the point of view of the BIP, software would be free to implement this proposal and not BIP 152, and just rely on getdata messages to request blocks. Acceptance of this proposed BIP does not require acceptance/implementation of BIP 152.

In the future, we can also save a bit of memory by not allocating (or deallocating) the peer data structures we maintain for transaction and address relay. Can't we do that today, just by not allocating it until we know the peer wants it?

As I mention above, the goal is to be able to bound a peer's resource utilization at the time the connection is completed. It would be unfortunate if a peer enabled tx relay later in a connection's lifetime and suddenly got disconnected just because we attempted to move it from one accounting bucket to another.

sdaftuar force-pushed on Jan 5, 2021

sdaftuar renamed this:
~~p2p: Add BLOCKRELAY message for negotiating block-relay-only connection~~
p2p: Add DISABLETX message for negotiating block-relay-only connections
on Jan 6, 2021

practicalswift commented at 7:06 PM on January 10, 2021: contributor

Concept ACK

sdaftuar force-pushed on Jan 12, 2021

sdaftuar force-pushed on Feb 4, 2021

in src/net_processing.cpp:1650 in e5e5fe80ad outdated

1644 | @@ -1630,7 +1645,8 @@ void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman&
1645 |  static void RelayAddress(const CNode& originator,
1646 |                           const CAddress& addr,
1647 |                           bool fReachable,
1648 | -                         const CConnman& connman)
1649 | +                         const CConnman& connman,
1650 | +                         PeerManagerImpl* peerman)

jonatack commented at 10:26 PM on February 5, 2021:

Could update the RelayAddress Doxygen documentation.

  * [@param](/bitcoin-bitcoin/contributor/param/)[in] connman Connection manager to choose nodes to relay to.
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] peerman ...
  */

sdaftuar commented at 4:37 PM on February 11, 2021:

Done.

in src/net_processing.cpp:2821 in e5e5fe80ad outdated

2816 | +            pfrom.fDisconnect = true;
2817 | +            return;
2818 | +        }
2819 | +        if (pfrom.m_tx_relay != nullptr && WITH_LOCK(pfrom.m_tx_relay->cs_filter, return pfrom.m_tx_relay->fRelayTxes)) {
2820 | +            // Can't send a DISABLETX message if they didn't turn off tx-relay
2821 | +            // in the version message.

jonatack commented at 10:28 PM on February 5, 2021:

Some doc suggestions following recent updates on master to the wtxidrelay and addrv2 documentation immediately above

+    // BIPXXX defines feature negotiation of disabletx, which must happen
+    // between VERSION and VERACK.
     if (msg_type == NetMsgType::DISABLETX) {
         if (pfrom.fSuccessfullyConnected) {
-            // Disconnect peers that send this message after VERACK; this
-            // must be negotiated between VERSION and VERACK.
+            // Disconnect peers that send a disabletx message after VERACK.
             pfrom.fDisconnect = true;
             return;
         }
         if (pfrom.IsFullOutboundConn()) {
-            // If we picked an outbound for tx-relay and it sends us a
+            // If we picked an outbound for transaction relay and it sends us a
             // disabletx, we want to find another peer.
             pfrom.fDisconnect = true;
             return;
         }
         if (pfrom.m_tx_relay != nullptr && WITH_LOCK(pfrom.m_tx_relay->cs_filter, return pfrom.m_tx_relay->fRelayTxes)) {
-            // Can't send a DISABLETX message if they didn't turn off tx-relay
-            // in the version message.
+            // Can't send a disabletx message if they didn't turn off
+            // transaction relay in the version message.
             pfrom.fDisconnect = true;

sdaftuar commented at 4:37 PM on February 11, 2021:

Done.

in src/net_processing.cpp:902 in e5e5fe80ad outdated

 896 | @@ -891,6 +897,15 @@ void PeerManagerImpl::PushNodeVersion(CNode& pnode, int64_t nTime)
 897 |      }
 898 |  }
 899 |  
 900 | +bool PeerManagerImpl::IsAddrRelayPeer(const CNode& pnode)
 901 | +{
 902 | +    PeerRef peer = GetPeerRef(pnode.GetId());

jonatack commented at 10:29 PM on February 5, 2021:

    const PeerRef peer = GetPeerRef(pnode.GetId());

sdaftuar commented at 4:36 PM on February 11, 2021:

Done.

in src/version.h:12 in e5e5fe80ad outdated

   8 | @@ -9,7 +9,7 @@
   9 |   * network protocol versioning
  10 |   */
  11 |  
  12 | -static const int PROTOCOL_VERSION = 70016;
  13 | +static const int PROTOCOL_VERSION = 70017;

jonatack commented at 10:30 PM on February 5, 2021:

static constexpr int PROTOCOL_VERSION = 70017;

sdaftuar commented at 4:36 PM on February 11, 2021:

Done.

in src/version.h:42 in e5e5fe80ad outdated

  37 | @@ -38,6 +38,9 @@ static const int INVALID_CB_NO_BAN_VERSION = 70015;
  38 |  //! "wtxidrelay" command for wtxid-based relay starts with this version
  39 |  static const int WTXID_RELAY_VERSION = 70016;
  40 |  
  41 | +//! "disabletx" message (eg used for block-relay-only connections) starts with this version
  42 | +static const int DISABLE_TX_VERSION = 70017;

jonatack commented at 10:31 PM on February 5, 2021:

static constexpr int DISABLE_TX_VERSION = 70017;

sdaftuar commented at 4:36 PM on February 11, 2021:

Done.

jonatack commented at 10:56 PM on February 5, 2021: member

Reviewed the changes and the BIP draft. Still looking if anything is missing and other tests that could be added.

Summary notes of the changes to compare with the BIP draft:

add DISABLE_TX_VERSION = 70017
bump PROTOCOL_VERSION to 70017
add DISABLETX protocol message

Updated message-handling in net_processing:

add std::atomic bool m_disable_tx{false}
updates in PeerManagerImpl::ProcessMessages:

VERSION, broadcast DISABLETX if eligible version and IsBlockOnlyConn
DISABLETX, disconnect if after VERACK, if IsFullOutboundConn or if fRelayTxes, otherwise set m_disable_tx to true
MEMPOOL, disconnect if m_disable_tx
FILTERLOAD/FILTERADD/FILTERCLEAR, disconnect if m_disable_tx

RelayAddrsWithConn essentially replaced by IsAddrRelayPeer that adds peer nullptr/m_disable_tx checks in:

RelayAddress
PeerManagerImpl::ProcessMessages ADDR, ADDRV2
PeerManagerImpl::SendMessages address refresh broadcast, addr

Here is a branch with a commit that updates p2p_addrv2_relay.py with your test: https://github.com/jonatack/bitcoin/commits/disabletx-addrv2-test

MarcoFalke commented at 10:47 AM on February 11, 2021: member

Needs rebase:

fuzz: test/fuzz/process_message.cpp:57: auto initialize_process_message()::(anonymous class)::operator()() const:
Assertion `"GetNumMsgTypes() == getAllNetMessageTypes().size()" && check' failed.

sdaftuar force-pushed on Feb 11, 2021

sdaftuar commented at 4:37 PM on February 11, 2021: member

@jonatack Thanks for the review, I included your commit that adds the addrv2 test as well.

sdaftuar marked this as ready for review on Feb 11, 2021

laanwj added this to the "Blockers" column in a project

pstratem commented at 7:29 PM on February 11, 2021: contributor

@sdaftuar We seem to have a number of p2p commands now that are effectively just telling the peer which commands we want to/support receiving. Maybe it's time for a command that just lists which commands we want/support?

sdaftuar commented at 7:33 PM on February 11, 2021: member

@pstratem I'm not opposed to that idea, but it seems to me that a change to the way feature negotiation works generally that incorporates existing p2p message negotiation could happen independently of this proposal, so I'd prefer not to gate this work on it. If a proposal like that is ready for deployment before this, I'd be happy to change the implementation here.

in src/net_processing.cpp:2974 in e142e81c51 outdated

2789 | @@ -2784,6 +2790,18 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
2790 |          return;
2791 |      }
2792 |  
2793 | +    if (msg_type == NetMsgType::DISABLETX) {
2794 | +        if (pfrom.fSuccessfullyConnected) {
2795 | +            // Disconnect peers that send this message after VERACK; this
2796 | +            // must be negotiated between VERSION and VERACK.
2797 | +            pfrom.fDisconnect = true;

MarcoFalke commented at 9:23 AM on February 12, 2021:

in the first commit:

The other negotiation message types have a NET-debug log statement preceding this disconnect. Is there any reason to not log to NET-debug as well here?

sdaftuar commented at 2:15 PM on February 12, 2021:

Done.

ajtowns commented at 5:43 AM on March 8, 2022:

There isn't a NET-debug comment for disconnecting peers when they're full-outbound or didn't set fRelay=false but nevertheless sent DISABLETX. I figured from this comment there would be...

in src/net_processing.cpp:1650 in aa09a205ca outdated

1646 |  static void RelayAddress(const CNode& originator,
1647 |                           const CAddress& addr,
1648 |                           bool fReachable,
1649 | -                         const CConnman& connman)
1650 | +                         const CConnman& connman,
1651 | +                         PeerManagerImpl* peerman)

MarcoFalke commented at 10:29 AM on February 12, 2021:

unrelated style nit in the second commit: instead of passing connman and peerman, this could be made a member function of peerman. The following diff on top of this commit compiles for me. Though, this can also be done in a separate pull.

diff --git a/src/net_processing.cpp b/src/net_processing.cpp
index e9584ddac1..84ac462099 100644
--- a/src/net_processing.cpp
+++ b/src/net_processing.cpp
@@ -258,6 +258,9 @@ public:
 
     /** Whether to relay addresses with a peer. */
     bool IsAddrRelayPeer(const CNode& pnode);
+    void RelayAddress(const CNode& originator,
+                      const CAddress& addr,
+                      bool fReachable);
 
 private:
     /** Consider evicting an outbound peer based on the amount of time they've been behind our tip */
@@ -1640,14 +1643,10 @@ void RelayTransaction(const uint256& txid, const uint256& wtxid, const CConnman&
  * [@param](/bitcoin-bitcoin/contributor/param/)[in] addr Address to relay.
  * [@param](/bitcoin-bitcoin/contributor/param/)[in] fReachable Whether the address' network is reachable. We relay unreachable
  * addresses less.
- * [@param](/bitcoin-bitcoin/contributor/param/)[in] connman Connection manager to choose nodes to relay to.
- * [@param](/bitcoin-bitcoin/contributor/param/)[in] peerman PeerManager implementation being used
  */
-static void RelayAddress(const CNode& originator,
-                         const CAddress& addr,
-                         bool fReachable,
-                         const CConnman& connman,
-                         PeerManagerImpl* peerman)
+void PeerManagerImpl::RelayAddress(const CNode& originator,
+                                   const CAddress& addr,
+                                   bool fReachable)
 {
     if (!fReachable && !addr.IsRelayable()) return;
 
@@ -1655,7 +1654,7 @@ static void RelayAddress(const CNode& originator,
     // Use deterministic randomness to send to the same nodes for 24 hours
     // at a time so the m_addr_knowns of the chosen nodes prevent repeats
     uint64_t hashAddr = addr.GetHash();
-    const CSipHasher hasher = connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24 * 60 * 60));
+    const CSipHasher hasher = m_connman.GetDeterministicRandomizer(RANDOMIZER_ID_ADDRESS_RELAY).Write(hashAddr << 32).Write((GetTime() + hashAddr) / (24 * 60 * 60));
     FastRandomContext insecure_rand;
 
     // Relay reachable addresses to 2 peers. Unreachable addresses are relayed randomly to 1 or 2 peers.
@@ -1664,7 +1663,7 @@ static void RelayAddress(const CNode& originator,
     std::array<std::pair<uint64_t, CNode*>,2> best{{{0, nullptr}, {0, nullptr}}};
     assert(nRelayNodes <= best.size());
 
-    auto sortfunc = [&best, &hasher, nRelayNodes, &originator, &addr, &peerman](CNode* pnode) {
+    auto sortfunc = [&best, &hasher, nRelayNodes, &originator, &addr, peerman = this](CNode* pnode) {
         if (peerman->IsAddrRelayPeer(*pnode) && pnode != &originator && pnode->IsAddrCompatible(addr)) {
             uint64_t hashKey = CSipHasher(hasher).Write(pnode->GetId()).Finalize();
             for (unsigned int i = 0; i < nRelayNodes; i++) {
@@ -1683,7 +1682,7 @@ static void RelayAddress(const CNode& originator,
         }
     };
 
-    connman.ForEachNodeThen(std::move(sortfunc), std::move(pushfunc));
+    m_connman.ForEachNodeThen(std::move(sortfunc), std::move(pushfunc));
 }
 
 void static ProcessGetBlockData(CNode& pfrom, Peer& peer, const CChainParams& chainparams, const CInv& inv, CConnman& connman)
@@ -2885,7 +2884,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
             if (addr.nTime > nSince && !pfrom.fGetAddr && vAddr.size() <= 10 && addr.IsRoutable())
             {
                 // Relay to a limited number of other nodes
-                RelayAddress(pfrom, addr, fReachable, m_connman, this);
+                RelayAddress(pfrom, addr, fReachable);
             }
             // Do not store addresses outside our network
             if (fReachable)

sdaftuar commented at 2:17 PM on February 12, 2021:

Done.

MarcoFalke approved

MarcoFalke commented at 10:50 AM on February 12, 2021: member

review ACK 953399d8ef6badf6f522ebef9b8e51f15ba2f259 I checked that this implements bip-disable-tx 🍎

<details><summary>Show signature and timestamp</summary>

Signature:

Timestamp of file with hash fca6ed1c697212896baf34608539b9c39bf9cc8ddf858743ef5db86218388168 -

</details>

MarcoFalke commented at 11:07 AM on February 12, 2021: member

I am wondering why it is allowed to send a disabletx message and then follow up with a tx message. See also https://github.com/bitcoin/bips/pull/1052/files#r575130294 which raises the same question in the bip. Even if the bip stays as is, we might want to disallow tx messages as well, or is there some downside I am missing?

<details><summary>The "test" I wrote</summary>

diff --git a/test/functional/p2p_filter.py b/test/functional/p2p_filter.py
index 458e5235b6..d8c6b256ef 100755
--- a/test/functional/p2p_filter.py
+++ b/test/functional/p2p_filter.py
@@ -18,6 +18,8 @@ from test_framework.messages import (
     msg_getdata,
     msg_mempool,
     msg_version,
+    msg_disabletx,
+    msg_tx,
 )
 from test_framework.p2p import P2PInterface, p2p_lock
 from test_framework.script import MAX_SCRIPT_ELEMENT_SIZE
@@ -205,14 +207,6 @@ class FilterTest(BitcoinTestFramework):
         self.nodes[0].disconnect_p2ps()
 
     def run_test(self):
-        filter_peer = self.nodes[0].add_p2p_connection(P2PBloomFilter())
-        self.log.info('Test filter size limits')
-        self.test_size_limits(filter_peer)
-
-        self.log.info('Test BIP 37 for a node with fRelay = True (default)')
-        self.test_filter(filter_peer)
-        self.nodes[0].disconnect_p2ps()
-
         self.log.info('Test BIP 37 for a node with fRelay = False')
         # Add peer but do not send version yet
         filter_peer_without_nrelay = self.nodes[0].add_p2p_connection(P2PBloomFilter(), send_version=False, wait_for_verack=False)
@@ -220,8 +214,14 @@ class FilterTest(BitcoinTestFramework):
         version_without_fRelay = msg_version()
         version_without_fRelay.nRelay = 0
         filter_peer_without_nrelay.send_message(version_without_fRelay)
+        filter_peer_without_nrelay.send_message(msg_disabletx())
         filter_peer_without_nrelay.wait_for_verack()
         assert not self.nodes[0].getpeerinfo()[0]['relaytxes']
+        assert self.nodes[0].getpeerinfo()[0]['bytesrecv_per_msg']['disabletx'] == 24
+        filter_peer_without_nrelay.send_message(msg_tx())
+        assert self.nodes[0].getpeerinfo()[0]['bytesrecv_per_msg']['tx'] == 34
+        filter_peer_without_nrelay.send_message(msg_filterclear())
+        assert self.nodes[0].getpeerinfo()[0]['bytesrecv_per_msg']['tx'] == 34
         self.test_frelay_false(filter_peer_without_nrelay)
         self.test_filter(filter_peer_without_nrelay)

</details>

sdaftuar commented at 1:24 PM on February 12, 2021: member

I am wondering why it is allowed to send a disabletx message and then follow up with a tx message. See also https://github.com/bitcoin/bips/pull/1052/files#r575130294 which raises the same question in the bip. Even if the bip stays as is, we might want to disallow tx messages as well, or is there some downside I am missing? @MarcoFalke The intent is to disallow sending a tx on such a link -- already it's the case that if we receive a transaction from a block-relay-only outbound peer that we'll disconnect them. Once we no longer instantiate the m_tx_relay structure for inbound peers sending us disabletx, the existing code would already disconnect such a peer that sends an unrequested tx as well; but I will go ahead and implement that behavior in this PR as well because that is indeed an oversight. Thanks for noticing this!

sdaftuar force-pushed on Feb 12, 2021

in src/net_processing.cpp:1667 in 62d0eaa3f7 outdated

1662 | @@ -1647,8 +1663,8 @@ static void RelayAddress(const CNode& originator,
1663 |      std::array<std::pair<uint64_t, CNode*>,2> best{{{0, nullptr}, {0, nullptr}}};
1664 |      assert(nRelayNodes <= best.size());
1665 |  
1666 | -    auto sortfunc = [&best, &hasher, nRelayNodes, &originator, &addr](CNode* pnode) {
1667 | -        if (pnode->RelayAddrsWithConn() && pnode != &originator && pnode->IsAddrCompatible(addr)) {
1668 | +    auto sortfunc = [&best, &hasher, nRelayNodes, &originator, &addr, peerman = this](CNode* pnode) {
1669 | +        if (peerman->IsAddrRelayPeer(*pnode) && pnode != &originator && pnode->IsAddrCompatible(addr)) {

jnewbery commented at 2:40 PM on February 12, 2021:

Is it possible to avoid this extreme coupling between the net and net_processing layers? Here, net_processing is constructing a callback (sortfunc) which it hands to net via ForEachNodeThen(). In that callback, net calls into a net_processing function (IsAddRelayPeer()), which in turn calls into a function in the net layer (RelayAddrsWithConn()). This net_processing -> net -> net_processing -> net layering makes it very difficult to construct a mental model of which component is responsible for what functions, and makes future work to rationalize the interface much more difficult.

MarcoFalke commented at 3:30 PM on February 12, 2021:

Haven't looked at the details, but I'd presume that RelayAddrsWithConn will be (can be) removed once the addr filter is moved to net processing and initialized only when needed.

sdaftuar commented at 3:55 PM on February 12, 2021:

Yeah I think this will get better once we finish moving things from net to net_processing. In this case, I've added a new variable and member function to net_processing (which is where I think it should live in the long run) but until the rest of it moves, we're going to have to look in both places for some logic. (I'm assuming we all agree that the logic around which addresses get relayed to which peers should live in net_processing, not net.)

jnewbery commented at 9:35 AM on February 14, 2021:

I don't agree with this line of logic. I think it'd be far better to remove complexity first and then implement this cleanly, rather than add on more complexity and hoping to remove it later. Also note that this change is a pessimization: it locks and releases m_peer_mutex for every peer (up to 125 times). My understanding is that the ForEachNode() helpers exist in part to avoid this kind of repeated locking/unlocking.

sdaftuar commented at 10:39 AM on February 14, 2021:

@jnewbery You've been doing the most work on moving things out of net -- can you be more specific about what you want to see in this implementation? I was trying to follow a guideline of "don't add new things to net if they should end up in net_processing", in the hopes of not making that work any harder. Are you suggesting that I add this to net to make this implementation cleaner, or wait for more of your refactoring work to land, or something else?

jnewbery commented at 11:32 AM on February 14, 2021:

Are you suggesting that I add this to net to make this implementation cleaner, or wait for more of your refactoring work to land, or something else?

Yes, certainly rebasing this on the https://github.com/jnewbery/bitcoin/tree/2020-06-cs-main-split branch from #19398 would remove this tight net-net_processing coupling. We all agree that we want addr relay logic to live entirely in net_processing. My point here is that changes we make now should be done in a way that moves in that direction, not in a way that makes it more difficult.

sdaftuar commented at 11:56 AM on February 14, 2021:

Thanks for being clear about your preference here. I don't think it's worth gating this work on all those commits landing in master, but if other reviewers prefer to see that work land first, then of course I'll rebase after your commits are merged.

For now I've gone ahead and simplified this PR by just putting m_disable_tx in CNode, which makes things no worse than they are today as far as the separation between net and net_processing (and indeed it simplifies this PR by not adding the complexity you pointed out here in RelayAddress).

sdaftuar force-pushed on Feb 14, 2021

in src/protocol.h:267 in ea3c2e22b4 outdated

 259 | @@ -260,6 +260,13 @@ extern const char* CFCHECKPT;
 260 |   * @since protocol version 70016 as described by BIP 339.
 261 |   */
 262 |  extern const char* WTXIDRELAY;
 263 | +/**
 264 | + * Indicates that a node will not relay transactions, and (for now) also
 265 | + * not relay addresses. This can be used by nodes implementing
 266 | + * block-relay-only connections.
 267 | + * @since protocol version 70017.

MarcoFalke commented at 11:58 AM on February 14, 2021:

nit: Could mention the BIP number, now that one was assigned?

sdaftuar commented at 6:34 PM on February 16, 2021:

Done.

DrahtBot added the label Needs rebase on Feb 17, 2021

sdaftuar force-pushed on Feb 17, 2021

DrahtBot removed the label Needs rebase on Feb 17, 2021

in src/net.h:513 in f03e93202d outdated

 505 | @@ -506,7 +506,12 @@ class CNode
 506 |          // Don't relay addr messages to peers that we connect to as block-relay-only
 507 |          // peers (to prevent adversaries from inferring these links from addr
 508 |          // traffic).
 509 | -        return m_conn_type != ConnectionType::BLOCK_RELAY;
 510 | +        if (m_conn_type == ConnectionType::BLOCK_RELAY) return false;
 511 | +
 512 | +        // Don't relay addr messages to peers that have sent disabletx, as they are
 513 | +        // likely to be inbound block-relay-only peers.
 514 | +        if (m_disable_tx) return false;

ajtowns commented at 2:44 AM on February 18, 2021:

Moving RelayAddrsWithConn to net_processing and making it:

static inline bool RelayAddrsWithConn(const Node& node, const Peer& peer) {
    return !node.IsBlockOnlyConn() && !peer.m_disable_tx;
}

would preserve the net/net_processing separation better, I think?

sdaftuar commented at 2:02 PM on February 18, 2021:

I think that without a bigger refactoring, John's observation that we call into ForEachNode but access data in net_processing would still remain. Given that we have a major refactoring effort underway (and I think #21186 will address this directly, from a quick glance), I'd rather leave efforts to refactor out of this PR -- if the refactors get merged first, I'll update the approach here.

jnewbery commented at 3:08 PM on February 18, 2021:

Suhas is right. I think until we move addr data into net_processing there are only bad and less bad approaches here. Leaving this in net to avoid having to make net_processing function calls in ForEachNode() seems like the less bad option to me (see #20726 (review)).

sdaftuar force-pushed on Feb 18, 2021

in src/net_processing.cpp:3233 in 9d822489c2 outdated

3229 | @@ -3199,7 +3230,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3230 |          // Stop processing the transaction early if
3231 |          // 1) We are in blocks only mode and peer has no relay permission
3232 |          // 2) This peer is a block-relay-only peer
3233 | -        if ((m_ignore_incoming_txs && !pfrom.HasPermission(PF_RELAY)) || (pfrom.m_tx_relay == nullptr))
3234 | +        if ((m_ignore_incoming_txs && !pfrom.HasPermission(PF_RELAY)) || (pfrom.m_tx_relay == nullptr) || pfrom.m_disable_tx)

ariard commented at 1:45 PM on February 23, 2021:

Should we also severe connections for block-relay-only peers sending us inv(tx) to be consistent with transaction message reception ?

Currently the BIP mentions that the following messages shouldn't be relayed between block-relay-only peers : {inv(tx), getdata(tx), getdata(merkleblock), filter, mempool, tx}. This PR implements disconnections for filter, mempool, tx but not for the remaining ones. Any rational for such disconnection selection ?

sdaftuar commented at 6:40 PM on March 9, 2021:

I don't know that it's worth disconnecting for getdata(merkleblock); it already seems to be the case that by default we permit any peer to send us a getdata(merkleblock) and we ignore it. As getdata(block) is always permitted, it seems like this is harmless to allow and ignore.

I'll update the PR to disconnect for inv(tx) and getdata(tx).

in src/net_processing.cpp:2963 in 9d822489c2 outdated

2854 | +            // Disconnect peers that send a disabletx after VERACK.
2855 | +            LogPrint(BCLog::NET, "disabletx received after verack from peer=%d; disconnecting\n", pfrom.GetId());
2856 | +            pfrom.fDisconnect = true;
2857 | +            return;
2858 | +        }
2859 | +        if (pfrom.IsFullOutboundConn()) {

ariard commented at 1:58 PM on February 23, 2021:

Should this check also includes IsAddrFetchConn() ?

A peer implementing the recommendation around disabling addr-relay won't provide us any addrs, devoiding such connection type on our side from its purpose.

Note, I don't think it's an issue if you have two peers of this software talking as m_disable_tx is only set by the DISABLETX receiver. Our node won't send a DISABLETX for such non-IsBlockOnlyConn, such not disable-tx on this link. Extending this check is more a measure encompassing other clients strictly implementing the BIP.

sdaftuar commented at 8:32 PM on March 3, 2021:

I think this is unnecessary for now, for two reasons:

it will be unusual to get a disabletx from an outbound peer. So given that, it's not clear what their behavior is if they send us a disabletx, and maybe they will respond to a getaddr anyway (which is permitted by the BIP).
I think the better behavior is for us to disconnect an addr fetch peer after some timeout if no addr message is received (seems like a bug that we don't do that right now). Once we fix that, then disconnecting early doesn't provide much benefit and just adds unnecessary complexity.

ajtowns commented at 5:47 AM on March 8, 2022:

Should this be testing pfrom.IsFullOutboundConn() && !IgnoresIncomingTxs() -- if you've set -blocksonly=1 there's less need to reject outbounds that set DISABLETX?

sdaftuar commented at 6:51 PM on March 8, 2022:

Such a node would still need addr messages, so I think it's safest to just disconnect for now if it's unexpected, and if we want to open it up later we can always come up with some policy for how many disabletx peers we're willing to tolerate in our spots reserved for full outbound peers.

Edit: I realized after I posted that the recent behavior changes we made around addr messages may eliminate the concern I raised -- but at any rate, I don't really want to add additional complexity that is gated on -blocksonly=1 mode, it just seems like such a special case and I don't think we get much benefit in this situation (where I don't expect anyone will be using disabletx on inbound connections for the time being).

in src/net_processing.cpp:2972 in 9d822489c2 outdated

2862 | +            pfrom.fDisconnect = true;
2863 | +            return;
2864 | +        }
2865 | +        if (pfrom.m_tx_relay != nullptr && WITH_LOCK(pfrom.m_tx_relay->cs_filter, return pfrom.m_tx_relay->fRelayTxes)) {
2866 | +            // Can't send a disabletx message if they didn't turn off
2867 | +            // transaction relay in the version message.

ariard commented at 2:38 PM on February 23, 2021:

I don't think this interaction with BIP37 is described in the new BIP. The following case "If a node sets the transaction relay field in the version message to a peer to true it MUST NOT send a disabletx message" isn't covered ?

Or do you consider our software to be more severe than the BIP and another implementation can decide to override BIP 37 tx-relay in version message ? It's maybe what you meaned with "This proposal is compatible with, but independent of, BIP 37. "

sdaftuar commented at 8:36 PM on March 3, 2021:

Ah, I didn't realize this was confusing, but yes I can make this clearer in the BIP. The language currently says

If a node sets the transaction relay field in the version message to a peer to false, then the disabletx message MAY also be sent in response to a version message from that peer if the peer's protocol version is >= 70017. If sent, the disabletx message MUST be sent prior to sending a verack.

My intention was that if you didn't specify fRelay, or if you set it to true, then you MUST NOT send a disabletx message.

Yes my thinking was that if node software implements BIP 37, then I didn't want this to conflict with BIP37-related logic, so I tried for compatibility. At the same time, it seemed to me that fRelay may be required by some implementations (BIP60 tried to make it so, though I don't know what other software has implemented) so I figured the simplest thing was just to require software implementing this BIP to also be sending fRelay, and therefore the value being sent should be false.

It would also be possible to completely disentangle this from BIP 37 and either ignore the fRelay field or only require it to be false if it's sent at all, but I don't know how much difference that makes. I think the way I've done it is clear and consistent (well once I fix the language in the BIP to remove the ambiguity you point out!) but other ways would be fine too.

ariard commented at 2:45 PM on March 15, 2021:

Receiver-side, we can enforce a consistency requirement between fRelay and DISABLETX to detect buggy upgraded peers signaling differing relay type preferences.

Sender-side, if we're connected to a non-upgraded peer (nVersion < 70017), we should send a fRelay, otherwise they can't discover our relay preferences. If we're connected to an upgraded peer, we can ignore setting fRelay in our version message and just send a DISABLETX.

I don't think we have specific considerations to avoid disruption BIP37-related logic, in the sense of upgrading the connection type to full-relay if they signal fRelay=false. Doing so would break the purpose of our block-relay-only strategy. Actually, that's what we already do, at VERSION message reception, we ignore fRelay if peer doesn't have a m_tx_relay struct.

Network-wise,, it'll be safe to deprecate BIP37-related logic on our side (i.e don't bother sending fRelay even to non-upgraded peers) only once BIP338 is widely deployed. Otherwise, legacy peers will be automatically disconnected when they're picked up as block-relay-only, thus impoverishing their connection graph (only able to connect as full-relay inbound slots).

Note, I think the BIP change should waiwe about the omission case, otherwise it make a requirement to implement BIP60 only for the sake of sending a fRelay=false, and thus create an interdependency between both standards ?

sdaftuar commented at 12:49 AM on January 5, 2022:

Sender-side, if we're connected to a non-upgraded peer (nVersion < 70017), we should send a fRelay, otherwise they can't discover our relay preferences. If we're connected to an upgraded peer, we can ignore setting fRelay in our version message and just send a DISABLETX.

I think we still want to send fRelay=false, because we don't know that an upgraded peer has implemented the BIP -- the p2p version bump is just to signify which p2p messages are considered valid. (For instance, some other new p2p message may be introduced for some other reason, which might cause someone to bump the p2p version they are advertising -- even if they haven't added support for BIP 338.)

So I think given that, we might as well treat fRelay as a required part of the spec for software that is implementing disabletx.

ariard commented at 11:19 PM on January 9, 2022:

Thanks to recall that p2p version advertising doesn't imply support of the well-known associated set of messages with that version number.

If so, I agree that if we're connected to an upgraded peer, we should send fRelay=false as an alternative mechanism to disable transaction relay messages on the connection. However, if we want to deprecate fRelay in the future once BIP338 is deployed, I think we should not treat fRelay as a required part of the spec.

To the best of my understanding, the proposed BIP338 draft is ambiguous as it sounds to make a requirement of a disabletx receiving peer to support first BIP37 ("(provided that BIP 37 or BIP 60 has been implemented") though at the same time explicitly says those standards are independent ("This proposal is compatible with, but independent of, BIP 37. ").

I would favor to make the BIP37 deprecation option more explicit in BIP338 (i.e remove "provided that BIP37...") and that dual-support is an implementation choice, not a spec requirement.

That said, this comment is not a substantial blocker to me to advance this work forward.

in src/net.h:511 in 9d822489c2 outdated

 505 | @@ -506,7 +506,12 @@ class CNode
 506 |          // Don't relay addr messages to peers that we connect to as block-relay-only
 507 |          // peers (to prevent adversaries from inferring these links from addr
 508 |          // traffic).
 509 | -        return m_conn_type != ConnectionType::BLOCK_RELAY;
 510 | +        if (m_conn_type == ConnectionType::BLOCK_RELAY) return false;
 511 | +
 512 | +        // Don't relay addr messages to peers that have sent disabletx, as they are

ariard commented at 2:44 PM on February 23, 2021:

Should the BIP recommendation around addr-relay updated to scope GETADDR ? Behavior should already be compliant on sender-side, we don't send getaddr to block-relay peers.

sdaftuar commented at 6:44 PM on March 9, 2021:

If the BIP were centered around block-relay-only behavior (as I had originally proposed), then I would think it would make sense to fully specify addr-related behavior, including getaddr.

However as the BIP is scoped more narrowly to just define disabletx, I think it's better to leave out getaddr. If a peer sends us a disabletx and a getaddr, I think we ought to just respond to it and assume the peer knows what it's doing.

ariard commented at 2:58 PM on February 23, 2021: member

Mostly comments around the BIP and the consistency of the new behavior. That said I found the new BIP version far clearer than the first draft, thanks for it.

From the BIP:

A node that has sent or received a disabletx message to/from a peer MUST NOT send any of these messages to the peer:

Maybe it should include notfound for transactions ?

This BIP's recommendations for software to not relay addresses is intended to be interpreted as guidance in the absence of any such future protocol extension, to accommodate existing software behavior.

At least, it could be argue that's also a bandwidth-saving for incoming block-relay-only peers. One they're not able to do now, beyond accommodating existing block-relay-only connection openers.

sdaftuar force-pushed on Mar 9, 2021

sdaftuar commented at 7:35 PM on March 9, 2021: member

Thanks for the review @ariard. I've drafted some small changes to the BIP to clarify the questions you raised; please take a look here and let me know if this answers all your questions, and I'll open a pull to the bip repo after that.

https://github.com/sdaftuar/bips/blob/2021-02-bip338-fixups/bip-0338.mediawiki

I added a fixup commit that adds additional restrictions to peers that send us disabletx; if this looks good I'll squash into place.

sdaftuar commented at 1:23 AM on March 10, 2021: member

Seems like we’re going in circles discussing this at length, with no consensus on how to move forward. I am happy for someone to implement this another way, if that is the preference — but I think this approach is superior to the alternate suggestions being made about reusing fRelay.

I’m closing this PR for now; please feel free to consider this up for grabs if anyone else is interested.

sdaftuar closed this on Mar 10, 2021

fanquake removed this from the "Blockers" column in a project

ariard commented at 2:51 PM on March 15, 2021: member

@sdaftuar, fixups sounds good to me. FWIW, see my comment on your proposed bip338 changes. I agree this approach is superior to the alternate suggestions. I hope some of the bip content around defining well the set of messages excluded in case of tx-relay disabling could be incorporated in some bip37/60 extension.

I don't plan to take this forward but I'll review it again if it gets back on track.

laanwj commented at 2:00 PM on March 16, 2021: member

Somewhat sad to see this closed. I think it made a lot of sense.

Rspigler commented at 3:10 AM on June 21, 2021: contributor

Mark up for grabs?

ryanofsky added the label Up for grabs on Jul 2, 2021

sdaftuar reopened this on Jan 4, 2022

sdaftuar force-pushed on Jan 4, 2022

MarcoFalke removed the label Up for grabs on Jan 4, 2022

in test/functional/p2p_disabletx.py:33 in 0e9bbd8b6b outdated

  28 | +    msg_tx,
  29 | +    msg_version,
  30 | +)
  31 | +from test_framework.p2p import (
  32 | +    P2P_VERSION,
  33 | +    P2P_SUBVERSION,

DrahtBot commented at 5:03 PM on January 4, 2022:

test/functional/p2p_disabletx.py:31:1: F401 'test_framework.p2p.P2P_SUBVERSION' imported but unused

�[1mWARNING!�[0m The following scripts are not being run: ['p2p_disabletx.py']. Check the test lists in test_runner.py.

sdaftuar force-pushed on Jan 4, 2022

sdaftuar commented at 12:51 AM on January 5, 2022: member

Reopened this PR as it still seems relevant. I've simplified the code some (now that addr relay was separately handled in #21528), and I believe I've addressed all of the review comments left before, particularly from @ariard's review. I've also added a test that should cover just about all the logic introduced in this PR.

I believe the BIP draft at https://github.com/sdaftuar/bips/blob/2021-02-bip338-fixups/bip-0338.mediawiki is still current for the changes in this PR (so please point out any discrepancies!).

sipa commented at 6:33 PM on January 5, 2022: member

Given the addr relay policy changes from #21528, do you think it's still useful to include the "disabletx -> don't relay addr" suggestion (currently rule 6) in the BIP?

I ask because I think the assumption underlying 21528 is that we can effectively perfectly guess which clients care about addr relay already, so it feels less needed to make that interact with BIP338.

sdaftuar commented at 7:14 PM on January 5, 2022: member

Given the addr relay policy changes from #21528, do you think it's still useful to include the "disabletx -> don't relay addr" suggestion (currently rule 6) in the BIP?

I ask because I think the assumption underlying 21528 is that we can effectively perfectly guess which clients care about addr relay already, so it feels less needed to make that interact with BIP338.

Well, the behavior we introduced in #21528 is not documented in any BIP or (I assume) standardized across other implementations; so I think on balance it still makes sense to include that recommendation for other software that may not have implemented that sort of logic. I guess the way I'm looking at this is #21528 is just a different (and broader) way for our software to comply with the recommendation in this BIP, which eliminates the need for this PR to do anything differently to comply with that recommendation.

Down the road, I'd still hope for addr relay policies to be standardized in a BIP and supersede the recommendation in this one.

in src/net_processing.cpp:3290 in 6c13af6ccc outdated

3287 | @@ -3262,7 +3288,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3288 |          // Stop processing the transaction early if
3289 |          // 1) We are in blocks only mode and peer has no relay permission
3290 |          // 2) This peer is a block-relay-only peer

ariard commented at 10:36 PM on January 9, 2022:

nit: "This is peer a block-relay-only peer/disable_tx peer"

sdaftuar commented at 5:20 PM on January 10, 2022:

Addressed in latest fixup commit.

in src/net_processing.cpp:327 in 6c13af6ccc outdated

 283 | @@ -284,6 +284,9 @@ struct Peer {
 284 |      /** Work queue of items requested by this peer **/
 285 |      std::deque<CInv> m_getdata_requests GUARDED_BY(m_getdata_requests_mutex);
 286 |  
 287 | +    /** Whether a peer has sent us a disabletx message */

mzumsande commented at 10:40 PM on January 9, 2022:

Since the wording of the BIP is symmetric with respect to sender/receiver ("A node that has sent or received a disabletx message to/from a peer MUST NOT send any of these messages to the peer") - would it make sense to also set m_disable_tx after sending out a disabletx message to a peer, so that the disconnect logic is also symmetrical?

While there would be no difference for some messages (TX, tx-INV) on which we'd already disconnect, that would mean we'd be more disconnect-happy when dealing with others (e.g. tx-GETDATA, NOTFOUND) which are currently ignored. I realize that we can't be certain that our peer has implemented BIP338 even if they use version >= 70017, but I wonder if they'd have any good reason sending us either of these messages even if they didn't?

sdaftuar commented at 4:28 PM on January 14, 2022:

The way I look at this issue is that we have two competing goals: a) Have well-defined behavior on the p2p network, to limit the range of behavior and make interactions easier to reason about in the future. This tends towards being strict around deploying new specifications, both to simplify our software logic and so that when people write their own implementations, they might notice if they break something during testing.

b) Avoid disconnecting peers unless it's important/we have a good reason, because otherwise, over time, the network graph will become more fragile and less partition-resistant: older software that is silently failing to stay connected to newer software will have fewer honest peers to connect to and become easier to sybil. (As an aside: to the extent that we do sometimes disconnect peers for imperfect behavior, it can be less risky to the network if we differentiate between inbound and outbound peers, and be more tolerant of inbound peers -- this would allow slightly broken software to still find honest peers and not be partitioned off the network over time.)

So with that in mind, I'm reluctant to add new disconnection semantics for behavior that is harmless; in this case, I think it is supposed to be harmless to not have implemented BIP 338, as this is meant to be a purely optional specification. The question to me then becomes: since we already disconnect peers for some messages if we have sent fRelay=0, should we add to the set of disallowed messages to such peers based on how we understand our own software to behave?

I think it's a good point that those messages (tx-getdata, tx-notfound) should never come through to us if we've sent a disabletx, because presuming that our software isn't itself broken, we would never announce or send transactions to the peer. However, if there is some other software on the network that is slightly broken around transaction relay and occasionally sends us a tx-getdata or tx-notfound when not appropriate, is that a big deal? I'm not sure...

First, changing our behavior in this way wouldn't prevent malicious peers from wasting our bandwidth (as they can do that without triggering disconnection, even without using these messages). So instead, it would be to catch/handle broken peers and prevent them from taking up a connection slot or wasting our bandwidth.

If there is broken software out there that we know has a bug like this which causes these fRelay=0 connections to be high bandwidth when they shouldn't be, by sending these specific p2p messages that you're suggestion we disconnect for, then I think that would be sufficient reason to do exactly what you suggest. However, I'm not sure that we're aware of any such software. In the absence of knowing such broken software exists, I think two other options are reasonable: a) do nothing until we notice it, or b) write smarter logic that catches the thing that I think we actually care about for these connections, which is bandwidth waste (ie seeing high traffic of bogus messages as a percentage of total traffic), and manages our connections using that metric as a factor.

So I'd say right now I'm reluctant, but not strongly opposed, to adding more disconnection criteria like you're suggesting in this PR. However if someone proposed adding more disconnection criteria as part of a bigger resource management strategy, I think that could be totally reasonable.

ajtowns commented at 6:15 AM on March 4, 2022:

"to the extent that we do sometimes disconnect peers for imperfect behavior, it can be less risky to the network if we differentiate between inbound and outbound peers, and be more tolerant of inbound peers" -- I like that idea, it means that if a connection gets closed for not being strictly conformant enough, it's the software that attempted the connection in the first place that closes the connection, and can reasonably include a debug log entry to indicate what's going on, without becoming intolerably spammy.

in src/net_processing.cpp:328 in 6c13af6ccc outdated

 283 | @@ -284,6 +284,9 @@ struct Peer {
 284 |      /** Work queue of items requested by this peer **/
 285 |      std::deque<CInv> m_getdata_requests GUARDED_BY(m_getdata_requests_mutex);
 286 |  
 287 | +    /** Whether a peer has sent us a disabletx message */
 288 | +    std::atomic<bool> m_disable_tx{false};

ariard commented at 10:50 PM on January 9, 2022:

I think the implementation is currently exhaustive in the reject of transaction relay messages.

The BIP could be preemptively updated to scope rejection of BIP330 messages, as those ones are transaction relay related. Though as #23443 is only implementing the Erlay connection-setting message (SENDRECON), I don't think we have interdependency in the PRs.

sdaftuar commented at 5:24 PM on January 10, 2022:

I think I'm inclined to hold off on that until Erlay is further along -- if they both go in for the next release, then I'll update the BIP to incorporate mention of it. But if Erlay or other future p2p extensions get merged down the road, I think it'll make more sense to specify in the BIPs for those features exactly how they interact with disabletx.

in src/protocol.h:269 in f58b7ac737 outdated

 265 | @@ -266,7 +266,7 @@ extern const char* WTXIDRELAY;
 266 |   * Indicates that a node will not relay transactions, and (for now) also
 267 |   * not relay addresses. This can be used by nodes implementing
 268 |   * block-relay-only connections.
 269 | - * @since protocol version 70017.
 270 | + * @since protocol version 70017 as described by BIP 338.

ariard commented at 10:54 PM on January 9, 2022:

The doc/bips.md can be also updated to mention BIP 338.

sdaftuar commented at 5:24 PM on January 10, 2022:

Done.

ariard commented at 11:32 PM on January 9, 2022: member

The code appears in sync with the latest state of the draft BIP. Still have to verify test coverage.

sdaftuar commented at 6:33 PM on January 10, 2022: member

If so, I agree that if we're connected to an upgraded peer, we should send fRelay=false as an alternative mechanism to disable transaction relay messages on the connection. However, if we want to deprecate fRelay in the future once BIP338 is deployed, I think we should not treat fRelay as a required part of the spec. @ariard Deprecating fRelay doesn't seem very likely to me as something we will try to do, just because it seems like it potentially breaks any software that may have implemented BIP 60 for basically no benefit. Maybe no such software exists, but it just doesn't seem worth the effort to try to determine whether that is true, particularly if getting it wrong would be hard to know and would have unfortunate consequences on the network graph (while getting it right has no real benefit).

So while I could change the BIP to be more agnostic, and just require fRelay=false if fRelay is sent at all, that seems less useful than just requiring fRelay to be sent instead. Note that no other parts of BIP 37 are required, so effectively this BIP would be just building off of BIP 60 -- which I think is the safest assumption about what will be compatible with other software on the network.

RandyMcMillan commented at 5:36 PM on January 12, 2022: contributor

Concept ACK

gruve-p commented at 11:54 AM on January 23, 2022: contributor

Concept ACK

in test/functional/p2p_disabletx.py:151 in 4792aac064 outdated

 144 | +        peer.wait_for_disconnect()
 145 | +
 146 | +    def notfound_after_disabletx_test(self):
 147 | +        self.log.info('Check for disconnection when sending a notfound(tx) message after disabletx')
 148 | +
 149 | +        for inv_type in (MSG_TX, MSG_WTX):

ariard commented at 12:12 AM on January 27, 2022:

Note, I wonder if we can send back a notfound(MSG_TX | MSG_WITNESS_FLAG) as those messages are replied to getdatas.

ariard commented at 12:22 AM on January 27, 2022: member

ACK 28df701

Verified the tests coverage.

@ariard Deprecating fRelay doesn't seem very likely to me as something we will try to do, just because it seems like it potentially breaks any software that may have implemented BIP 60 for basically no benefit. Maybe no such software exists, but it just doesn't seem worth the effort to try to determine whether that is true, particularly if getting it wrong would be hard to know and would have unfortunate consequences on the network graph (while getting it right has no real benefit).

While I think there is a minimal benefit to prune out old p2p logic, at least to decrease future maintenance costs, I agree that we don't have a formalized p2p features deprecation process, minimizing disruption risks for external softwares silently relying on the fRelay behavior.

sdaftuar force-pushed on Jan 27, 2022

sdaftuar commented at 4:35 PM on January 27, 2022: member

Thanks @ariard; I've squashed that fixup commit and pushed the cleaned up branch.

ariard commented at 1:12 AM on February 9, 2022: member

ACK 625b9ae

Changes since last ACK, fixup commit squash.

ghost commented at 3:35 PM on February 10, 2022: none

Concept ACK

ajtowns commented at 7:01 AM on March 8, 2022: member

Approach ACK, with comments :) I'd like to see this merged.

Wording: The "update bips.md" commit calls it "bips.mid". The main description of the PR could probably use some updates -- this isn't a draft, it could link to the bips repo now, and it's already got plenty of concept acks?

Comments on other comments: I think it would only make sense to deprecate fRelay as part of switching to a new "p2p v2" protocol.

Rest is I guess more about the bip/spec than the code per se.

The bip currently specifies "A node that has sent or received a disabletx message to/from a peer MUST NOT send any of these messages to the peer:"

I think it would be better for that to be asymmetric, that is to say:

a node that has received a disabletx message from a peer (m_disable_tx == true) MUST NOT send INV/NOTFOUND messages for tx data, or TX messages to a peer; and
a node that has sent a disabletx message to a peer (IsBlockOnlyConn()) MUST NOT send GETDATA for tx/merkleblock data, FILTERADD/FILTERLOAD/FILTERCLEAR or MEMPOOL messages.

In that case, if an inbound blocks only node sends you a disabletx message, you can avoid allocating the TxRelay structure for that node saving memory, and also save bandwidth by avoiding sending INVs etc to that node; but if that inbound node is running in -blocksonly=1 mode, it can still send the occasional tx to you which you can relay on to the rest of the network.

Letting -blocksonly=1 nodes free up resources seems worth the slight extra complexity to me, but ymmv I guess.

NOTFOUND behaviour isn't mentioned in the bip.

I guess "It is RECOMMENDED [you don't send addr/getaddr/addrv2 if you sent/received disabletx]" is stronger than I'd choose; and I don't think it's exactly what we implement, but overall agree with sdaftuar's comment.

mzumsande commented at 3:56 PM on March 8, 2022: member

In that case, if an inbound blocks only node sends you a disabletx message, you can avoid allocating the TxRelay structure for that node saving memory, and also save bandwidth by avoiding sending INVs etc to that node; but if that inbound node is running in -blocksonly=1 mode, it can still send the occasional tx to you which you can relay on to the rest of the network.

I'm not sure I understand your idea: Does this mean you are suggesting that DISABLETX should be sent on all non-whitelisted outgoing connections if a node is in -blocksonly mode? That would probably mean that the address relay recommendation would need be dropped, because -blocksonly nodes want to participate in it.

Letting -blocksonly=1 nodes free up resources seems worth the slight extra complexity to me, but ymmv I guess.

It would help the peers of -blocksonly=1 nodes free up resources (by not having to initialize m_tx_relay for this peer), but not the -blocksonly=1 nodes themselves?

sdaftuar force-pushed on Mar 8, 2022

sdaftuar commented at 7:28 PM on March 8, 2022: member

@ajtowns I don't think that -blocksonly=1 peers should try to use disabletx, because part of the reason that disabletx connections are low-bandwidth is that compact blocks still work on these links -- each node should just be getting transactions from elsewhere. If you're really running in -blocksonly=1 mode, and in the future when I hope we account for disabletx peers separately, then I think it's better to use up a full connection slot for your peer to reflect the higher expected bandwidth.

I also don't like the idea of adding ambiguity about whether transactions might relay over a disabletx connection. We're not making -blocksonly=1 peers any worse off with this change, and I don't see a need to try to special case those semantics in new behavior that is essentially unrelated -- the goal is to make an allowance for lightweight block-relay-only peers, where we know the behavior is that only blocks and headers should propagate.

The updated BIP draft is here: https://github.com/bitcoin/bips/pull/1287

I've fixed the typo in the commit message you mentioned, added additional logging, and also changed the bips.md update to reference 24.0 (and not 23.0) as the first release to include this code (hoping that's a good assumption!).

ajtowns commented at 9:01 AM on March 9, 2022: member

Hmm, I think m_disable_tx should be exposed via GetNodeStateStats/getpeerinfo?

I don't think that -blocksonly=1 peers should try to use disabletx, because part of the reason that disabletx connections are low-bandwidth is that compact blocks still work on these links -- each node should just be getting transactions from elsewhere. If you're really running in -blocksonly=1 mode, and in the future when I hope we account for disabletx peers separately, then I think it's better to use up a full connection slot for your peer to reflect the higher expected bandwidth.

In that case, should -blocksonly=1 nodes not send DISABLETX for their 2 additional blocks-only connections? Currently, those connections will use up as much of their peers' bandwidth as their full outbound connections (except for addr messages). (I think it still makes sense to have the blocks-only connections, both for the short-lived scans to check the network hasn't been partitioned, and for the long-lived pair of blocks-only connections that don't relay addr messages so are hard for malicious third parties to discover) If we bump the defaults up to 4 or 6 blocks-only connections, that would mean 20% of -blocksonly=1 traffic going via blocks-only connections would become 33% or 43%.

Should DISABLETX also forbid BLOCK/GETDATA(block) messages in that case?

But... I'm still not sure you couldn't reasonably consider -blocksonly=1 connections to be low-bandwidth in the first place? They don't need any of the tx INV traffic, and the block traffic at least could be distributed amongst all their peers; so if you can go from 10 outbound peers to 14 outbound peers that's reducing per-peer block traffic from 200MB/week to 144MB/week (at 2MB/block). I don't know; I was thinking the motivation was mostly about avoiding having to allocate large data structures like the bloom filter or avoiding complex work like dealing with erlay reconciliation sets.

(And in particular, I guess I'd like to avoid allocating the bloom filter/doing reconciliation sets for all the -blocksonly=1 connections in addition to blocks-only connections)

(When we have some slots reserved for low-bandwidth blocksonly connections, perhaps we should also set NODE_NETWORK_LIMITED to prevent them from being used for IBD)

sdaftuar commented at 1:22 AM on March 10, 2022: member

But... I'm still not sure you couldn't reasonably consider -blocksonly=1 connections to be low-bandwidth in the first place? They don't need any of the tx INV traffic, and the block traffic at least could be distributed amongst all their peers; so if you can go from 10 outbound peers to 14 outbound peers that's reducing per-peer block traffic from 200MB/week to 144MB/week (at 2MB/block). I don't know; I was thinking the motivation was mostly about avoiding having to allocate large data structures like the bloom filter or avoiding complex work like dealing with erlay reconciliation sets.

I guess I'm not really sure how best to think about this case. I do think that bandwidth is perhaps the least important network resource to worry about (memory seems very important, as does CPU). I think the goal is to make it so that in non-adversarial situations, the additional resources used by adding additional DISABLETX connection slots is something that node operators wouldn't really notice -- so that there's no issue with adding additional block-relay-only connections to the network graph.

I think -blocksonly=1 peers are somewhere in between; they should allow for less memory but also more bandwidth if they are sometimes relaying full blocks and addrs. Maybe that's fine as long as the CPU usage is still low?

At any rate, I assume -blocksonly=1 is a relatively uncommon mode, so I'm not sure how important this is to optimize for. If we changed the semantics to not relay transactions at all (or offered a mode where that was the case) then it's probably fine to start using DISABLETX on all the connections for such a peer. With the current behavior, I also think it's fine for -blocksonly=1 peers to have their regular connections be treated the same as (say) a node with an infinite minrelayfee/feefilter setting.

Should DISABLETX also forbid BLOCK/GETDATA(block) messages in that case?

I do think it's important not to ban GETDATA(block) traffic on block-relay-only links, because the whole point is to serve as an emergency fallback to keep the network in sync if there's some kind of attack happening. The way I think about this is that we want the normal, non-adversarial case to be very low total network overhead for having these connections (memory, bandwidth, cpu, etc) but in the adversarial case, it's fine if node resource usage might go up a bit because then we're also getting an important benefit (and you'd figure that having defenses against such attacks makes it unappealing to try).

ajtowns commented at 3:35 AM on March 10, 2022: member

At any rate, I assume -blocksonly=1 is a relatively uncommon mode,

Yeah, I think that might be the key point. Probably -blocksonly=1 isn't that attractive in general: if you want to be quick to get blocks, you want to get txs and do compact blocks; if you want to not use much resources, you probably want headers only, or to tunnel info directly rather than be on the open p2p network; if you want to just minimise requirements for a full p2p node, you probably just want a smaller but non-zero mempool... Provided -blocksonly=1 nodes are only a very small proportion of the network, it doesn't seem super worthwhile optimising for the resources they use on other nodes. And if we did want to optimise for them, we could have them only have a couple of connections for relaying their own txs, and have all the rest of them be no-txs-ever connections which would let them use DISABLETX.

sdaftuar commented at 10:25 PM on March 10, 2022: member

Updated the OP as @ajtowns suggested, and added a commit to expose Peer::m_disable_tx via getpeerinfo() (and updated the test to cover the new code).

ajtowns commented at 8:02 PM on March 16, 2022: member

Running a couple of nodes on signet with this patch, seems to work.

I guess for now, if you do getpeerinfo, then all the nodes that say "disabletx_received": true and have been connected for more than a few minutes have all chosen you as a blocks-only anchor. Don't think you can really do much with that info, though (and you could've gotten it previously by working out if they'd set relaytxes=false and not started addr relay I think).

DrahtBot added the label Needs rebase on Mar 25, 2022

Add "disabletx" feature negotiation message support

When we pick an outbound peer to be a block-relay-only peer, try to let the
peer know via the "disabletx" network message that we will not relay
transactions, so that the peer can allocate resources effectively and avoid
sending us addr messages.

Note: in this commit we just ignore the disabletx command when received.

724b05ca02

sdaftuar force-pushed on Mar 28, 2022

DrahtBot removed the label Needs rebase on Mar 28, 2022

in src/net_processing.cpp:4179 in 41e82573f7 outdated

4170 | @@ -4118,6 +4171,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4171 |              LOCK(::cs_main);
4172 |              for (CInv &inv : vInv) {
4173 |                  if (inv.IsGenTxMsg()) {
4174 | +                    if (peer->m_disable_tx) {

jnewbery commented at 8:48 AM on March 30, 2022:

BIP 338 doesn't specify that a peer that sends a notfound(tx|wtx|witness_tx) after sending disable_tx should be disconnected. Should the BIP be updated to specify that (or the behaviour here be changed to just drop the message)?

ajtowns commented at 9:43 AM on March 30, 2022:

https://github.com/bitcoin/bips/pull/1287

jnewbery commented at 9:06 AM on March 30, 2022: member

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?

sdaftuar commented at 12:23 PM on April 1, 2022: member

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.

Add restrictions on peers that send us disabletx

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)

12a8da10a5

doc: protocol version 70017 is described in BIP 338 70b9d2886f

Add support to test framework for disabletx message cfbe9b71e7

test_framework: add support for failed outbound connections 799e3b0d7f

Tests for new disabletx p2p message

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.

4a74bce9fc

doc: Update bips.md for BIP 338 0114902d0f

rpc: expose disabletx status in getpeerinfo() 791048b712

sdaftuar force-pushed on Apr 5, 2022

sdaftuar commented at 1:21 PM on April 5, 2022: member

Updated this PR to restrict feefilter messages from disabletx peers. See also https://github.com/bitcoin/bips/pull/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 m_tx_relay object for the peer, so I think an additional restriction will be redundant (and isn't strictly required by the BIP). (Note that we won't send FEEFILTER messages to peers to whom we have sent a DISABLETX message.)

in src/net_processing.cpp:4141 in 791048b712

4137 | @@ -4085,6 +4138,11 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
4138 |      }
4139 |  
4140 |      if (msg_type == NetMsgType::FEEFILTER) {
4141 | +        if (peer->m_disable_tx) {

ariard commented at 6:44 PM on April 11, 2022:

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.

ariard commented at 6:45 PM on April 11, 2022: member

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 ?

ajtowns commented at 11:42 PM on April 11, 2022: member

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 ?

I think the idea is just that after disabletx is supported, we'll change the code so that TxRelay/m_tx_relay is only allocated for inbound connections after verack completes without disabletx having been received.

jnewbery commented at 5:25 PM on April 12, 2022: member

that behavior will automatically change, based on the current logic, once we stop instantiating a m_tx_relay object for the peer

I was looking at this in the context of #22778. It seems to me that m_fee_filter_sent and m_next_send_feefilter should probably be moved out of the TxRelay data structure. All of the other members of TxRelay are related to sending transactions to the peer, whereas m_fee_filter_sent and m_next_send_feefilter are both related to receiving transactions from the peer. A node's tx relay behaviour is not always symmetrical (eg a blocksonly node will ignore incoming transactions, but may still send out its own transactions), so I don't think we should be grouping that behaviour together in a single structure. What do you think?

ajtowns commented at 5:56 PM on April 12, 2022: member

I was looking at this in the context of #22778. It seems to me that m_fee_filter_sent and m_next_send_feefilter should probably be moved out of the TxRelay data structure. All of the other members of TxRelay are related to sending transactions to the peer, whereas m_fee_filter_sent and m_next_send_feefilter are both related to receiving transactions from the peer. A node's tx relay behaviour is not always symmetrical (eg a blocksonly node will ignore incoming transactions, but may still send out its own transactions), so I don't think we should be grouping that behaviour together in a single structure. What do you think?

disabletx is defined as being always symmetrical, and if the only time we drop TxRelay is when we send/receive disabletx and thus won't send/receive feefilter messages, it doesn't make a difference. See the conversation around #20726 (comment) ; my conclusion at least was that we don't want to consider -blocksonly=1 peers low bandwidth (because unlike block_relay_only peers they won't already have the txs to do compact block reconstruction), so do want to keep limiting them, so the extra memory usage from the TxRelay struct isn't a problem.

Otherwise, I think renaming the TxRelay struct to be TxSending and dropping those because they're only relevent to receiving txs would make sense, but then it would be another structure to make optional for disabletx peers, which seems like make-work.

ariard commented at 4:37 PM on April 13, 2022: member

I think the idea is just that after disabletx is supported, we'll change the code so that TxRelay/m_tx_relay is only allocated for inbound connections after verack completes without disabletx having been received.

Yes, I'm aligned on the direction to remove TxRelay for inbound connections devoid of tx-relay traffic. I'm just pointing that with current code I understand that a disabletx-upgraded node should disconnect outbound block-relay-only peers as those ones can still send FEEFILTER, am I incorrect ?

sdaftuar commented at 6:35 PM on April 13, 2022: member

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 ?

Here's how I think it works:

Suppose node A sends disabletx to node B. -> Now if nodeA sends a feefilter message to node B, B will disconnect A; this requirement that A must not send feefilter messages is now specified in the BIP.
Node B can still (optionally) send feefilter messages to node A, as the BIP does not prohibit this. -> If node A disconnected node B for sending feefilter, then that would effectively make node A incompatible with software that didn't implement BIP 338, which was not the intent of the BIP.
In this PR, nodes will only send disabletx to outbound block-relay-only peers, and we do not instantiate m_tx_relay for those block-relay-only peers. -> This is sufficient for node A to not send feefilter messages to node B, and hence node A should never be disconnected from node B due to the requirement that nodes sending disabletx not send feefilter messages.

ajtowns commented at 10:39 PM on April 13, 2022: member

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 fRelay=false, but you do send a FEEFILTER message to them, and they then disconnect you.

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" ?

sdaftuar commented at 3:33 PM on April 14, 2022: member

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 disabletx message that don't exist today for recipients of an fRelay=false message, but nevertheless I think your idea of having both parties that understand/implement BIP 338 to indicate as much by both sending a message to the other makes a lot of sense.

I'll rework the BIP and this code to implement that, thank you!

ariard commented at 4:23 PM on April 15, 2022: member

In this PR, nodes will only send disabletx to outbound block-relay-only peers, and we do not instantiate m_tx_relay for those block-relay-only peers. -> This is sufficient for node A to not send feefilter messages to node B, and hence node A should never be disconnected from node B due to the requirement that nodes sending disabletx not send feefilter messages.

Thanks, I understand my confusion. The feefilter prohibition is enforced by the disabletx receiver, in that case the outbound block-relay-only peer.

sdaftuar commented at 1:33 PM on May 13, 2022: member

Updated bip text is here: https://github.com/sdaftuar/bips/commit/ecf86e1e21706176b8e0b06e7b0a5f5e993bb720. Will update this branch next.

DrahtBot added the label Needs rebase on May 19, 2022

DrahtBot commented at 10:21 AM on May 19, 2022: member

🐙 This pull request conflicts with the target branch and needs rebase.

<sub>Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".</sub>

sdaftuar commented at 12:47 PM on May 19, 2022: member

Closing now that #22778 has been merged.

sdaftuar closed this on May 19, 2022

DrahtBot locked this on May 19, 2023