On Thu, Jan 14, 2021 at 1:46 AM Anthony Towns <aj@erisian.com.au> wrote:
> ==Specification==
> # A new disabletx message is added, [...]
> # A node that has sent or received a disabletx message to/from a peer MUST NOT send any of these messages to the peer:
> ## inv messages for transactions
> [...]
> # It is RECOMMENDED that a node that has sent or received a disabletx message to/from a peer not send any of these messages to the peer:
> ## addr/getaddr
> ## addrv2 (BIP 155)

In particular, I think combining these doesn't make sense for:

 * nodes running with -blocksonly (that want to stay up to date with
   the blockchain, but don't care about txes)
    - not sending disabletx reduces their potential connectivity, if
      nodes are willing to accept more disabletx peers due to the lower
      resource usage
    - sending disabletx means they can't maintain their addr db and find
      other nodes to connect to

This is quite implementation specific, but the Bitcoin Core implementation of -blocksonly already differs from what you would get if all outbound connections were treated as block-relay-only peers; I initially tried to make the -blocksonly setting behave the same as block-relay-only connections and never relay transactions, but it turned out that Bitcoin Core currently allows users running in -blocksonly mode to sometimes relay transactions themselves (there's even a test for it [1]).

This isn't compatible with the design of disabletx, so I don't think it could be used.  I think that is okay, as I don't think we need to make allowances right now to encourage the use of more -blocksonly nodes on the network by prioritizing their existing outbound connections[2]. In the case of block-relay-only connections, the next step I plan to propose in the Bitcoin Core implementation is an increase in the number of inbound slots to accommodate additional disabletx peers, to facilitate an eventual increase in the number of outbound block-relay-only connections that Bitcoin Core would initiate by default (and reduce potential concern about overwhelming the network's capacity).

 * non-listening nodes running with -connect to one/more preselected peers
    - they can't send disabletx generally because they want txes
    - they don't need addr information (since they only make connections
      to some known peers), and don't have many peers to relay addresses
      on to, so are essentially blackholes, so would like to disable
      addr relay for much the same reasons

While I think it's a valid use-case if someone wants to run nodes in this way, I don't see why it has direct bearing on this discussion -- adding protocol support for this use case is not incompatible with my disabletx proposal; indeed, once we have worked out an addr relay protocol that supports a peer telling another to not relay addresses, I would plan to use it along with disabletx.

-----

There was some discussion about this proposal on IRC recently as well [3] and this issue of mentioning addr-relay in a BIP about tx-relay seems to have drawn some critique.  I think if there were some other use case that other developers have which would use disabletx (say, because the current fRelay solution also is inadequate for those use cases today), yet would want addr relay on these links, then that would be a good reason to drop mention of it from the BIP and defer discussion of addr relay entirely to some new future proposal.  However, I'm not yet aware of such a use case, as the -blocksonly one mentioned above does not actually fit (and nor do I think that a -blocksonly mode where transactions are never sent but addr messages are exchanged is necessary beyond how we already do that today, by just sending fRelay=false).

Moreover, because there is no specification/BIP that governs how address relay should work on the network, I think that if we just dropped mention of addr-relay from the BIP, that software ought to just disable addr-relay to nodes sending disabletx, anyway!  Addr relay is currently purely local policy, so I think it's reasonable to conclude that if the only known use case of a peer sending disabletx is for a behavior that also would drop addr messages, then optimizing software for that use case is logical and I would propose that myself for how Bitcoin Core behaves.  It seems polite to mention this in the BIP as well, so that other software implementors have the same understanding that I have for how this should work, until we have an address relay protocol extension that governs it explicitly.

-----

Why not simply add a "disableaddr" type of message now?  I am not opposed to someone championing an addr relay protocol, but I personally think it is premature.  I'm not aware of any research or shared understanding of what the goals of address relay are or ought to be, particularly as they pertain to how addresses for obscure networks ought to propagate to nodes that may or may not support those networks.  Moreover, I'm not aware of any research into what relay strategies make sense to achieve any particular relay goals we might come up with.  I imagine that we will someday propose a way for software to communicate support for various network types (as defined in BIP 155), and that will relate to the kinds of relay policies we expect software on the network to be running.  But I think any p2p message we add now to simply "disableaddr" would be made redundant by a more extensive protocol in the future, so I don't feel comfortable defending a proposal to add such a message at this time, when it seems to be unnecessary for today's use cases.

So to summarize my view on this BIP's recommendation against sending addr messages to disabletx peers: if there is some software that someone plans to deploy soon that would seek to use disabletx but would prefer addr relay on these links, then I think that would cause me to re-evaluate this proposal to figure out the best way to achieve both use cases.  For now I think the recommendation I've proposed still makes sense.

--Suhas

[1] https://github.com/bitcoin/bitcoin/blob/f91587f050d9dceb45fe10129a76a4a9a060a09c/test/functional/p2p_blocksonly.py#L36

[2] Perhaps confusingly, running in -blocksonly mode is largely irrelevant to the outbound peering of such a node, as it makes 10 outbound connections by default, 8 of which are considered "full-relay" yet send fRelay=false to not receive inbound transaction traffic, and 2 considered "block-relay-only", also setting fRelay=false, but also not ever announcing transactions on those links.  I anticipate that in the future, Bitcoin Core software running in -blocksonly mode will continue to treat those 8 "full-relay" connections the same as today, and just start sending the disabletx message on the connections it considers to be truly block-relay-only (the two existing connections, along with any additional ones we later add).

[3] https://bitcoin.jonasschnelli.ch/ircmeetings/logs/bitcoin-core-dev/2021/bitcoin-core-dev.2021-01-12-15.00.log.html