Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

p2p: Add 2 outbound block-relay-only connections #15759

Merged
merged 9 commits into from Sep 7, 2019

Conversation

@sdaftuar
Copy link
Member

commented Apr 5, 2019

Transaction relay is optimized for a combination of redundancy/robustness as well as bandwidth minimization -- as a result transaction relay leaks information that adversaries can use to infer the network topology.

Network topology is better kept private for (at least) two reasons:

(a) Knowledge of the network graph can make it easier to find the source IP of a given transaction.

(b) Knowledge of the network graph could be used to split a target node or nodes from the honest network (eg by knowing which peers to attack in order to achieve a network split).

We can eliminate the risks of (b) by separating block relay from transaction relay; inferring network connectivity from the relay of blocks/block headers is much more expensive for an adversary.

After this commit, bitcoind will make 2 additional outbound connections that are only used for block relay. (In the future, we might consider rotating our transaction-relay peers to help limit the effects of (a).)

@gmaxwell

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

I agree with the concept. I wish we could announce our own addresses to these peers, but I think doing so results in a topology leak. :( The reason I'd like this is that if an attacker eclipses a node except for its blocks only links it will eventually eclipse its blocks-only links too through control of the addrman state.

@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

This approach was motivated by the TxProbe paper. Fundamentally, transaction relay is going to leak information about our node's peers (as long as we care about not wasting bandwidth). Rather than combat transaction-relay based inferences, which I think is a lost cause in the long-run even if there are improvements we can make in the short-term, I figure we can directly address why we care about keeping the graph private.

Some considerations I had while working on this:

  • Is 2 the right number of blocks-only connections? From a robustness standpoint, more connections like this are better; however there's a tradeoff with resource utilization. Two resources that occurred to me to worry about were the memory used by each CNode object (hence the refactor commits that allow the transaction-relay data structures to not be instantiated for the blocks-only peers), and the number of connection slots on the network (I figure going from 8 to 10 is not that bad?). I also did some simulations to verify that a random graph of 10000 peers would be fully connected if each peer makes 2 outbound connections to random other peers, which I thought was a good justification for 2 being a useful number of outbounds to add.

  • How should addrman interactions work on these blocks-only connections? My approach attempts to leave addrman unchanged as a result of these connections, neither processing addr messages nor sending any, to avoid leaking information via addr messages that we relay to other peers. But I have not given a lot of thought to what kind of attacks are possible using addrman, nor what the consequences are to not updating addrman with information from our blocks-only peers.

@gmaxwell

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

I also did some simulations to verify that a random graph of 10000 peers would be fully connected if each peer makes 2 outbound connections to random other peers,

Can you run these simulations assuming x% of the 10,000 peers are black holes (don't connect the graph), for various values of x%?

2 might be fine for now, but ultimately I'd like to optimize memory usage for both inbound and outbound blocksonly links, then at least double the inbound connection limit for blocks only links, and make 8 blocks only links. (I suspect running that simulation assuming reasonable number of black hole peers will show that 2 is not really enough).

@instagibbs

This comment has been minimized.

Copy link
Member

commented Apr 5, 2019

I figure we can directly address why we care about keeping the graph private

To the uninformed p2p reader this reason is?

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #16762 (Rust-based Backup over-REST block downloader by TheBlueMatt)
  • #16702 (p2p: supplying and using asmap to improve IP bucketing in addrman by naumenkogs)
  • #16698 ([WIP] Mempool: rework rebroadcast logic to improve privacy by amitiuttarwar)
  • #16682 ([WIP] p2p: Disconnect peer that send us tx INVs when we opted out of tx relay by jnewbery)
  • #16507 (feefilter: Compute the absolute fee rather than stored rate by instagibbs)
  • #15502 (Speed up initial connection to p2p network by ajtowns)
  • #14033 (p2p: Drop CADDR_TIME_VERSION checks now that MIN_PEER_PROTO_VERSION is greater by Empact)

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.

@gmaxwell

This comment has been minimized.

Copy link
Contributor

commented Apr 5, 2019

To the uninformed p2p reader this reason is?

(b) Knowledge of the network graph could be used to split a target node or
nodes from the honest network (eg by knowing which peers to attack in order to
achieve a network split).

@DrahtBot DrahtBot added the P2P label Apr 5, 2019
@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Apr 8, 2019

Can you run these simulations assuming x% of the 10,000 peers are black holes (don't connect the graph), for various values of x%?

This table shows my simulation results for various black hole rates, with various values of how many outbound connections we make (k=2 is what I've implemented in this PR). This is on a 10,000 node graph with no non-listening nodes. Each table entry shows the success count (where success is defined as "the subgraph of non-black-hole nodes is connected") out of 1000 simulations.

black hole rate k=2 k=4 k=6 k=8
0 1000 1000 1000 1000
5% 30 998 1000 1000
10% 0 976 1000 1000
15%   867 999 1000
20%   598 994 1000
25%   229 978 1000
30%   33 928 998
35%   2 812 992

2 might be fine for now, but ultimately I'd like to optimize memory usage for both inbound and outbound blocksonly links, then at least double the inbound connection limit for blocks only links, and make 8 blocks only links.

This is roughly what I have in mind as well. I think we need a p2p protocol change to allow negotiating blocks-only links (so that the peer on the other side of our connection knows that the inbound peer won't turn transaction relay on later), so I figured we could start small for now.

@practicalswift

This comment has been minimized.

Copy link
Member

commented Apr 8, 2019

@sdaftuar Thanks for sharing the results of the analysis! Would you be willing to share the simulation scripts?

@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Apr 9, 2019

@practicalswift My simulation code is unreadable, and also may have bugs, so I'd prefer that anyone interested in this would independently corroborate my results from scratch.

@naumenkogs

This comment has been minimized.

Copy link
Contributor

commented Apr 10, 2019

separating block relay from transaction relay;

This is indeed a good idea. I also see it as a small step towards peer rotation, I think this suggestion makes it easier to reason about rotation.

I'm not sure if this is strongly related to the data you provided, but I'm also curious how many nodes have to be attacked (suspended?) to split network in halves etc.

@luke-jr

This comment has been minimized.

Copy link
Member

commented Apr 18, 2019

Wouldn't this break all the benefits of compact blocks? (Ignoring that with the current implementation, block relay would likely still occur on the normal connections.)

@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Apr 18, 2019

@luke-jr I don't think so. The idea is that nodes are still expected to do transaction relay with other peers, and my belief is that transaction propagation is good enough that compact block relay works just fine on these blocksonly links. I believe my testing of this branch confirms that as well, but I'd welcome additional testers to corroborate those results in case I'm overlooking something.

@practicalswift

This comment has been minimized.

Copy link
Member

commented May 16, 2019

Concept ACK: I like the idea

@laanwj laanwj added this to Blockers in High-priority for review May 16, 2019
@sdaftuar sdaftuar force-pushed the sdaftuar:2019-03-blocksonly-edges branch from 7402e34 to 1b90f9c May 17, 2019
@DrahtBot DrahtBot removed the Needs rebase label May 17, 2019
Copy link
Member

left a comment

Maybe commits up to 7f2ce0a could be in a separate pull request.

src/net.h Outdated Show resolved Hide resolved
src/net.h Outdated
int64_t nNextAddrSend GUARDED_BY(cs_sendProcessing){0};
int64_t nNextLocalAddrSend GUARDED_BY(cs_sendProcessing){0};
struct AddrRelay {
CCriticalSection cs_addrsend;

This comment has been minimized.

Copy link
@promag

promag May 18, 2019

Member

c36939c

Could explain why the new mutex? This commit isn't just refactor.

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jun 5, 2019

Author Member

I think one issue is that I wasn't sure how to do the GUARDED_BY annotations with a variable that didn't live in the class. But also I thought it might make sense to explicitly lock things in this class with its own mutex, as that seems like a clearer design for making this code modular.

The reason I included this refactor commit at all was in preparation for being able to optionally not instantiate this data structure, if there was concern about minimizing resource utilization as much as possible for these additional blocksonly connections. Since I ended up not doing that (I believe I concluded that it probably wasn't all that much additional memory?), I could also drop this commit instead.

This comment has been minimized.

Copy link
@promag

promag Jun 11, 2019

Member

Thanks for the explanation, I'll review again with that in mind.

src/net.h Outdated Show resolved Hide resolved
src/net_processing.cpp Outdated Show resolved Hide resolved
src/net.h Outdated Show resolved Hide resolved
@jonasschnelli

This comment has been minimized.

Copy link
Member

commented May 18, 2019

Concept ACK.
I'm still trying to understand this more detailed: does that mean that with 2 block-only connection and an assumed 5% black-hole-nodes rate, we only have a 3% chance to succeed? Since we relay, I guess there is no chance we could identify (and thus disconnect) black-hole peers?

};

AddrRelay m_addr_relay;
const bool m_addr_relay_peer;

This comment has been minimized.

Copy link
@Empact

Empact May 27, 2019

Member

nit: m_addr_relay_peer could be private

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jun 5, 2019

Author Member

Meh, going to leave this as-is.

src/net.cpp Outdated Show resolved Hide resolved
src/net.h Show resolved Hide resolved
src/net.cpp Outdated Show resolved Hide resolved
@@ -1777,6 +1777,7 @@ bool AppInitMain(InitInterfaces& interfaces)
connOptions.nLocalServices = nLocalServices;
connOptions.nMaxConnections = nMaxConnections;
connOptions.nMaxOutbound = std::min(MAX_OUTBOUND_CONNECTIONS, connOptions.nMaxConnections);
connOptions.nMaxOutboundBlocksOnly = std::min(MAX_BLOCKS_ONLY_CONNECTIONS, std::max(connOptions.nMaxOutbound-6, 0));

This comment has been minimized.

Copy link
@dongcarl

dongcarl May 28, 2019

Contributor

Trying to understand the logic here...

According to the commit description, bitcoind will make 2 additional outbound connections, but with this logic, if connOptions.nMaxOutbound <= 6 then there will be no additional outbound connections: https://www.wolframalpha.com/input/?i=min(2,+max(x-6,+0))

From elsewhere in this commit, it seems like we consider the accounting for nMaxOutboundBlocksOnly as separate from that of nMaxOutbound, so perhaps just

 connOptions.nMaxOutboundBlocksOnly = MAX_BLOCKS_ONLY_CONNECTIONS;

would do?

This comment has been minimized.

Copy link
@EthanHeilman

EthanHeilman Jun 3, 2019

Contributor

This - 6 is a little confusing and requires that you look up the default value of connOptions.nMaxOutbound to understand what this is doing. Also echoing @dongcarl comments that this doesn't work well when connOptions.nMaxOutbound <= 6.

Given that this fails for particular values and you only want 2 block only outbound why not change this to:

connOptions.nMaxOutboundBlocksOnly = MAX_BLOCKS_ONLY_CONNECTIONS

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jun 5, 2019

Author Member

Perhaps the accounting logic should be improved to be clearer, but I believe the behavior here is at least (roughly) correct. The way this works is that if a user sets -maxconnections very low, that will be a hard limit on the number of inbound, outbound and outbound-blocks-only connections. Before this PR, it would take the user setting their max connections value to less than 8 to change the number of outbound connections we make (we first reduce all the inbound we take and only then reduce the outbound).

This PR adds new blocks-only connections and preferences those connections over regular outbound connections (whenever we make an outbound connection, we check to see if we are below our blocks-only target, and if so we initiate the connection as blocks-only). As a result, I thought it didn't make sense to have any blocks-only links if we have too few regular outbound connections, because I don't think it makes sense to have blocks-only links at the expense of transaction relay.

In particular, the problem with the suggestion of just setting nMaxOutboundBlocksOnly to 2 is that if the user passes -maxconnections=2, then they'll only have 2 blocksonly peers and won't receive any transactions.

So I believe the way I implemented things here is that we try to reserve 6 connections as regular outbound, and then if we add additional connections we will have those be blocks-only until we get to 2, and then any further connections are regular outbounds up to 8, and then finally any additional ones are used for feelers and inbounds.

Of course we could change this to be more conservative and only allocate blocks-only connections if we have all 8 regular outbounds. However I don't think we should be less conservative and risk having fewer regular outbounds.

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Jul 10, 2019

Contributor

I think just do std::min(MAX_BLOCKS_ONLY_CONNECTIONS, std::max(connOptions.nMaxConnections - MAX_OUTBOUND_CONNECTIONS, 0)).

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Jul 12, 2019

Author Member

@TheBlueMatt I pushed a commit that changes the behavior so that the first 8 outbounds are reserved to be regular full-relay peers, and only then do we allocate up to 2 blocksonly peers.

src/net.h Outdated Show resolved Hide resolved
src/init.cpp Outdated Show resolved Hide resolved
@sdaftuar sdaftuar force-pushed the sdaftuar:2019-03-blocksonly-edges branch from c408a7b to bacfdab Jun 5, 2019
@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Rebased and addressed review comments.

@DrahtBot DrahtBot removed the Needs rebase label Jun 5, 2019
@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Jun 11, 2019

I'm still trying to understand this more detailed: does that mean that with 2 block-only connection and an assumed 5% black-hole-nodes rate, we only have a 3% chance to succeed? Since we relay, I guess there is no chance we could identify (and thus disconnect) black-hole peers?

@jonasschnelli With 2 blocks-only connections, and if we assume 5% of nodes are useless black holes, then my simulation suggests we have a 3% chance of the network graph being connected by only the blocks-only connections. For additional robustness, we will want to add more blocks-only connections in the future -- but for resource utilization reasons I don't want to do that until we add better support at the p2p protocol layer for establishing these connections.

Note that we are still going to relay blocks on the existing 8 outbound connections, so this PR is purely additive robustness, at the cost of a little more resource utilization. We now will have 10 outbound peers, rather than 8, which are able to provide blocks to us. (We just aren't going to do tx relay on the two additional connections.)

One issue I'd like to flag for reviewers is how these new blocksonly peers interact with outbound peer disconnection (introduced in #11560 and #11490). I believe I've implemented this so that the new blocksonly peers will not be subject to other forms of disconnection, because non-tx-relay outbounds are excluded from being outbound disconnection candidates. However, it's worth verifying the implementation is correct and worth thinking about whether this behavior is the best choice.

@JeremyRubin

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

I think that this should be correct code in case someone wants to set it up to tabulate against Suhas's simulation.

Edit: I've rewritten it to be much much better. Scipy is so cool!

 import random
 import copy
 import scipy
 import scipy.sparse
 import scipy.linalg
 import numpy as np
 
 N = 1000
 def biased_coin(bias):
     return random.random() <= bias
 def make_graph(n_outbound, p_blackhole, n_total = N):
     graph = np.zeros((n_total, n_total))
     for i in range(n_total):
         if not biased_coin(p_blackhole):
             for j in range(n_outbound):
                 graph[i][j] = 1
             # rejection sample no self-edges
             while True:
                 np.random.shuffle(graph[i])
                 if not graph[i][i]:
                     break
     return graph
 
def is_connected(graph):
      return scipy.sparse.csgraph.connected_components(graph, directed=True, return_labels=False) == 1

 print(sum(is_connected(make_graph(8, 0.05, 1000)) for x in range(100)))

My results were, for 100 samples, 92. This suggests Suhas's simulation was not correct.

edit: Nevermind, Suhas's simulation was on n=10,000. will re-run

@JeremyRubin

This comment has been minimized.

Copy link
Contributor

commented Jun 11, 2019

Improved the code for the above sim, in case anyone's following by email-only.

@sdaftuar sdaftuar force-pushed the sdaftuar:2019-03-blocksonly-edges branch from c6f41a0 to b21b291 Aug 29, 2019
@sdaftuar sdaftuar changed the title p2p: Add 2 outbound blocks-only connections p2p: Add 2 outbound block-relay-only connections Aug 29, 2019
@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Aug 29, 2019

Fixed the p2p_blocksonly.py test to match the new behavior.

@ajtowns

This comment has been minimized.

Copy link
Contributor

commented Sep 2, 2019

* I don't fully understand the reasoning behind the _Don't relay addr messages to block-relay-only peers_.

I think I get this now.

I think the logic is that tx relay will allow an attacker to identify our tx relay peers, attack them, and eventually control all our tx relay connections; but if that happens we'll still relay blocks via our block relay connections. What we don't want is for the addr messages to potentially reveal which nodes we've selected for block relay to the attacker who by now controls all our other peers.

However an attacker that's connected to a large proportion of the entire network can identify your peers (or near-peers) by sending tainted addresses to you -- you then tell them to your peers and those peers then report back to the attacker identifying themselves. The patch here stops you from reporting tainted addresses from your blocks only peer (you ignore ADDR messages from them and won't send a GETADDR message), and stops you sending potentially tainted addresses to your blocks only peers (you don't choose them as the best peer in RelayAddress and even if you did you won't send addr messages to them in SendMessages).

I think this might degrade addr connectivity a little bit though -- RelayAddress won't choose your block-relay outbound connection, but it might choose an inbound peer that thinks of you as a block-relay connection, so will drop your messages into the void. If this change is widely deployed, this will be 20% of your inbound connections (or more if we eventually go from 8+2 to 8+8 or similar), so could be significant. I think since we AdvertiseLocal regularly to our non-block-relay peers, we could just set a flag on the peer if we've ever seen an ADDR message from them, and only choose nodes with that flag in RelayAddress?

@@ -2470,9 +2485,11 @@ bool static ProcessMessage(CNode* pfrom, const std::string& strCommand, CDataStr
if (strCommand == NetMsgType::TX) {
// Stop processing the transaction early if
// We are in blocks only mode and peer is either not whitelisted or whitelistrelay is off
if (!g_relay_txes && !pfrom->HasPermission(PF_RELAY))
// or if this peer is supposed to be a block-relay-only peer
if ((!g_relay_txes && !pfrom->HasPermission(PF_RELAY)) || (pfrom->m_tx_relay == nullptr))

This comment has been minimized.

Copy link
@ajtowns

ajtowns Sep 2, 2019

Contributor

This doesn't match the fBlocksOnly test for ::INV -- if it did it would be:

((!g_relay_txes || pfrom->m_tx_relay == nullptr) && !pfrom->HasPermission(PF_RELAY))

I think the logic here is probably better, though maybe there should be a check to prevent choosing a node for blocks-relay if it would have PF_RELAY perms?

This comment has been minimized.

Copy link
@sdaftuar

sdaftuar Sep 3, 2019

Author Member

I believe it's impossible for an outbound peer to be whitelisted and have PF_RELAY permissions, so these are the same...

This comment has been minimized.

Copy link
@ajtowns

ajtowns Sep 3, 2019

Contributor

I think you're right; HasPermission() checks m_permissionFlags and the only way m_permissionFlags is anything but PF_NONE is via AcceptConnection(), so inbound only.

@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Sep 3, 2019

I think this might degrade addr connectivity a little bit though -- RelayAddress won't choose your block-relay outbound connection, but it might choose an inbound peer that thinks of you as a block-relay connection, so will drop your messages into the void. If this change is widely deployed, this will be 20% of your inbound connections (or more if we eventually go from 8+2 to 8+8 or similar), so could be significant. I think since we AdvertiseLocal regularly to our non-block-relay peers, we could just set a flag on the peer if we've ever seen an ADDR message from them, and only choose nodes with that flag in RelayAddress?

I think the addr relay logic may need an overhaul, but I think the change made here is minor -- as I understand it, since we relay addr messages by choosing among all our inbounds as potential next hops, there's likely already a black-hole problem where light clients, spy nodes, adversaries, etc may be receiving addr messages and not propagating them further.

So adding 2 extra inbounds that don't participate in addr propagation should be a relatively minor effect. I just checked one of my listening nodes running a recent release of Bitcoin Core, and observed that out of 46 total connections, 18 have never sent me an addr message, and 24 have never sent me a getaddr message. I would surmise that at least those 18-24 peers do not participate in addr relay (and possibly more)... So I think the 20% estimate of addr-relay-degradation due to this PR is a large overestimate.

I do think your suggestion is likely to be some kind of an improvement (at least in the non-adversarial case) but I'd prefer to defer addr-relay improvements to a future PR.

sdaftuar added 6 commits Mar 8, 2019
Transaction relay is primarily optimized for balancing redundancy/robustness
with bandwidth minimization -- as a result transaction relay leaks information
that adversaries can use to infer the network topology.

Network topology is better kept private for (at least) two reasons:

(a) Knowledge of the network graph can make it easier to find the source IP of
a given transaction.

(b) Knowledge of the network graph could be used to split a target node or
nodes from the honest network (eg by knowing which peers to attack in order to
achieve a network split).

We can eliminate the risks of (b) by separating block relay from transaction
relay; inferring network connectivity from the relay of blocks/block headers is
much more expensive for an adversary.

After this commit, bitcoind will make 2 additional outbound connections that
are only used for block relay. (In the future, we might consider rotating our
transaction-relay peers to help limit the effects of (a).)
We don't want relay of addr messages to leak information about
these network links.
If we set fRelay=false in our VERSION message, and a peer sends an INV or TX
message anyway, disconnect. Since we use fRelay=false to minimize bandwidth,
we should not tolerate remaining connected to a peer violating the protocol.
@sdaftuar sdaftuar force-pushed the sdaftuar:2019-03-blocksonly-edges branch from b21b291 to 0ba0802 Sep 4, 2019
Copy link
Contributor

left a comment

re-utACK 0ba0802. Pointed out that stats.fRelayTxes was sometimes uninitialized for blocksonly peers (though its not a big deal and only effects RPC), which has since been fixed here. Otherwise changes are pretty trivial so looks good.


// Note that if we receive a getdata for a MSG_TX or MSG_WITNESS_TX from a
// block-relay-only outbound peer, we will stop processing further getdata
// messages from this peer (likely resulting in our peer eventually

This comment has been minimized.

Copy link
@TheBlueMatt

TheBlueMatt Sep 4, 2019

Contributor

Note that this comment is incomplete, we'll stop processing all messages from this peer (including pongs, so we'll eventually disconnect them as well).

This comment has been minimized.

Copy link
@jnewbery

jnewbery Sep 4, 2019

Member

I agree that we should update this comment (see #15759 (comment))

Copy link
Member

left a comment

utACK 0ba0802


// Note that if we receive a getdata for a MSG_TX or MSG_WITNESS_TX from a
// block-relay-only outbound peer, we will stop processing further getdata
// messages from this peer (likely resulting in our peer eventually

This comment has been minimized.

Copy link
@jnewbery

jnewbery Sep 4, 2019

Member

I agree that we should update this comment (see #15759 (comment))

@jamesob
jamesob approved these changes Sep 6, 2019
Copy link
Member

left a comment

ACK 0ba0802

Re-reviewed all commits. Cloned locally, built, and synced a few hundred blocks near the tip. Let the client run for most of the day. Found that getpeerinfo's bytes(sent|recv)_per_msg for peers with relaytxes=false compares as expected to full relay peers.

Blocks-only peer 1

    "bytessent_per_msg": {
      "getdata": 6396,
      "getheaders": 3159,
      "headers": 1272,
      "ping": 5472,
      "pong": 5472,
      "sendcmpct": 66,
      "sendheaders": 24,
      "verack": 24,
      "version": 127
    },
    "bytesrecv_per_msg": {
      "addr": 7660,
      "block": 121277168,
      "feefilter": 32,
      "getheaders": 1053,
      "headers": 3448,
      "ping": 5472,
      "pong": 5472,
      "sendcmpct": 66,
      "sendheaders": 24,
      "verack": 24,
      "version": 126
    }

Blocks-only peer 2

    "bytessent_per_msg": {
      "getdata": 5969,
      "getheaders": 2106,
      "headers": 742,
      "ping": 5440,
      "pong": 5472,
      "sendcmpct": 66,
      "sendheaders": 24,
      "verack": 24,
      "version": 127
    },
    "bytesrecv_per_msg": {
      "addr": 10975,
      "block": 111063475,
      "feefilter": 32,
      "getheaders": 1053,
      "headers": 4009,
      "ping": 5472,
      "pong": 5440,
      "sendcmpct": 66,
      "sendheaders": 24,
      "verack": 24,
      "version": 126
    }

Full-relay peer

    "bytessent_per_msg": {
      "addr": 15415,
      "feefilter": 32,
      "getaddr": 24,
      "getblocktxn": 953,
      "getdata": 475098,
      "getheaders": 1053,
      "headers": 106,
      "inv": 1919584,
      "notfound": 2501,
      "ping": 5472,
      "pong": 5472,
      "sendcmpct": 99,
      "sendheaders": 24,
      "tx": 325453,
      "verack": 24,
      "version": 127
    },
    "bytesrecv_per_msg": {
      "addr": 38192,
      "block": 107235996,
      "blocktxn": 889600,
      "cmpctblock": 572593,
      "feefilter": 32,
      "getdata": 29315,
      "getheaders": 1053,
      "headers": 106,
      "inv": 1362767,
      "notfound": 13058,
      "ping": 5472,
      "pong": 5472,
      "sendcmpct": 66,
      "sendheaders": 24,
      "tx": 5666974,
      "verack": 24,
      "version": 126
    }

I think this is ready for merge (perhaps after a signoff from @ajtowns?). I know I've harped on this before, but the risk-reward ratio here seems exceptionally low.

@@ -1753,7 +1753,8 @@ bool AppInitMain(InitInterfaces& interfaces)
CConnman::Options connOptions;
connOptions.nLocalServices = nLocalServices;
connOptions.nMaxConnections = nMaxConnections;
connOptions.nMaxOutbound = std::min(MAX_OUTBOUND_CONNECTIONS, connOptions.nMaxConnections);
connOptions.m_max_outbound_full_relay = std::min(MAX_OUTBOUND_FULL_RELAY_CONNECTIONS, connOptions.nMaxConnections);
connOptions.m_max_outbound_block_relay = std::min(MAX_BLOCKS_ONLY_CONNECTIONS, connOptions.nMaxConnections-connOptions.m_max_outbound_full_relay);

This comment has been minimized.

Copy link
@jamesob

jamesob Sep 6, 2019

Member

Way easier to reason about than the first iteration of this change. 👍

@sipa

This comment has been minimized.

Copy link
Member

commented Sep 6, 2019

ACK 0ba0802

@ajtowns

This comment has been minimized.

Copy link
Contributor

commented Sep 7, 2019

ACK 0ba0802 -- code review, ran tests. ran it on mainnet for a couple of days with MAX_BLOCKS_ONLY_CONNECTIONS upped from 2 to 16 and didn't observe any unexpected behaviour: it disconnected a couple of peers that tried sending inv's, and it successfully did compact block relay with some block relay peers.

fanquake added a commit that referenced this pull request Sep 7, 2019
0ba0802 Disconnect peers violating blocks-only mode (Suhas Daftuar)
937eba9 doc: improve comments relating to block-relay-only peers (Suhas Daftuar)
430f489 Don't relay addr messages to block-relay-only peers (Suhas Daftuar)
3a5e885 Add 2 outbound block-relay-only connections (Suhas Daftuar)
b83f51a Add comment explaining intended use of m_tx_relay (Suhas Daftuar)
e75c39c Check that tx_relay is initialized before access (Suhas Daftuar)
c4aa2ba [refactor] Change tx_relay structure to be unique_ptr (Suhas Daftuar)
4de0dba [refactor] Move tx relay state to separate structure (Suhas Daftuar)
26a93bc Remove unused variable (Suhas Daftuar)

Pull request description:

  Transaction relay is optimized for a combination of redundancy/robustness as well as bandwidth minimization -- as a result transaction relay leaks information that adversaries can use to infer the network topology.

  Network topology is better kept private for (at least) two reasons:

  (a) Knowledge of the network graph can make it easier to find the source IP of a given transaction.

  (b) Knowledge of the network graph could be used to split a target node or nodes from the honest network (eg by knowing which peers to attack in order to achieve a network split).

  We can eliminate the risks of (b) by separating block relay from transaction relay; inferring network connectivity from the relay of blocks/block headers is much more expensive for an adversary.

  After this commit, bitcoind will make 2 additional outbound connections that are only used for block relay. (In the future, we might consider rotating our transaction-relay peers to help limit the effects of (a).)

ACKs for top commit:
  sipa:
    ACK 0ba0802
  ajtowns:
    ACK 0ba0802 -- code review, ran tests. ran it on mainnet for a couple of days with MAX_BLOCKS_ONLY_CONNECTIONS upped from 2 to 16 and didn't observe any unexpected behaviour: it disconnected a couple of peers that tried sending inv's, and it successfully did compact block relay with some block relay peers.
  TheBlueMatt:
    re-utACK 0ba0802. Pointed out that stats.fRelayTxes was sometimes uninitialized for blocksonly peers (though its not a big deal and only effects RPC), which has since been fixed here. Otherwise changes are pretty trivial so looks good.
  jnewbery:
    utACK 0ba0802
  jamesob:
    ACK 0ba0802

Tree-SHA512: 4c3629434472c7dd4125253417b1be41967a508c3cfec8af5a34cad685464fbebbb6558f0f8f5c0d4463e3ffa4fa3aabd58247692cb9ab8395f4993078b9bcdf
@fanquake fanquake merged commit 0ba0802 into bitcoin:master Sep 7, 2019
2 checks passed
2 checks passed
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@fanquake fanquake removed this from Blockers in High-priority for review Sep 7, 2019
@MarcoFalke

This comment has been minimized.

@MarcoFalke

This comment has been minimized.

Copy link
Member

commented Oct 4, 2019

Maybe just copy-paste the section from the optech newsletter?

@sdaftuar

This comment has been minimized.

Copy link
Member Author

commented Oct 4, 2019

@MarcoFalke I added a little text to that wiki page referencing this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.