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

Net: Bucketing INV delays (1 bucket) for incoming connections to hide tx time #13298

Closed
wants to merge 1 commit into
base: master
from

Conversation

@naumenkogs
Copy link

naumenkogs commented May 21, 2018

It has been brought up to my attention that current random delays mechanism (originally intended to obfuscate transaction metadata) allows to easily estimate the time a transaction was received by a node.

It may be done by connecting multiple observer nodes to the same node. Each of those nodes will generate its own schedule of delays. Combined metadata regarding those events from different sources allows an observer to estimate transaction time.

After this patch a spy won't gain additional information by just creating multiple connections to a target.

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented May 21, 2018

I'm glad to see someone working on this!

One prior proposal was to put all inbound into one timing bucket, and keep per-peer timing for each outbound with the rationale that the attacker doesn't control our outbounds but could potentially control any inbounds.

If we found that to be too slow we could quantize inbound network groups into a small number of buckets (e.g. 4, by computing hasher(seed, netgroup)&3 or similar) and have timers per that so even when an attacker goes and gets a lot of groups his speedup has a small bound

I think probably we shouldn't be too afraid of slowing down propagation-- the changes to add delays at all were a massive slowdown and generated basically no notice.

Also related to this we might want to track the rate of novel transactions being accepted for a particular peer and up-delay really slow peers (like send every Nth time it would otherwise send). The rational being that relay of novel transactions blinds tracking (since we won't relay something a peer gave to us back to them) and also lazy tracking stuff tends not to relay anything. Delaying tx relay to a peer that never sends anything would hardly hurt propagation in the network, but at least might force attackers to be a bit more useful.

src/net.cpp Outdated
uint64_t net_group = pto->nKeyedNetGroup;
std::pair<int, uint64_t> key{average_interval_seconds, net_group};
LOCK(cs_group_next_sends);
std::map<std::pair<int, uint64_t>, int64_t>::iterator group_next_send = group_next_sends.find(key);

This comment has been minimized.

@gmaxwell

gmaxwell May 21, 2018

Member

I think for IPv6 this could be a slightly far-fetched memory exhaustion DOS.

@skeees

This comment has been minimized.

Copy link
Member

skeees commented May 21, 2018

Maybe I'm missing something, but what would be the downside of having a single next send across all outbound peers if propagation latency isn't a huge issue? It seems like delay by net group is moving in that direction

@sipa

This comment has been minimized.

Copy link
Member

sipa commented May 21, 2018

We assume that outgoing connections are less likely to be (significantly) under attacker control, due to them being self-selected, and addrman having built-in precautions against eclipse attacks (cc @EthanHeilman). Giving different peers a synchronized timer results increases bandwidth spikes, which isn't so much a concern for tx propagation, but may interfere with block propagation if they coincide. For outgoing peers (max 16) this is not that much of a concern, but then again, due to the previous point, synchronizing things is also less useful there.

Regarding @gmaxwell's point of memory exhaustion, we could introduce a per-group reference counter, and delete the corresponding map entries when no peers in that group remain?

src/net.h Outdated
@@ -310,6 +310,10 @@ class CConnman
unsigned int GetReceiveFloodSize() const;

void WakeMessageHandler();

/** Attempts to obfuscate tx time through exponentially distributed emitting. */
int64_t PoissonNextSendTo(int64_t now, int average_interval_seconds, CNode* pto);

This comment has been minimized.

@Empact

Empact May 21, 2018

Member

const CNode*, alternatively pass the net_group

src/net.h Outdated
@@ -434,6 +438,9 @@ class CConnman
* This takes the place of a feeler connection */
std::atomic_bool m_try_another_outbound_peer;

std::map<std::pair<int, uint64_t>, int64_t> group_next_sends GUARDED_BY(cs_group_next_sends);

This comment has been minimized.

@Empact

Empact May 21, 2018

Member

nit: include <map>

@naumenkogs

This comment has been minimized.

Copy link

naumenkogs commented May 21, 2018

@gmaxwell thanks for the feedback.

I like the idea of turning it into a small number of buckets.
I think that 8 buckets may be fine considering current load (should not cause significant spikes in bandwidth).

Please let me know if you see a better estimation for a reasonable number of buckets.

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented May 21, 2018

@skeees Putting all in one will result in more spiky bandwidth usage (e.g. you hit your queue then use a bunch of bandwidth transmitting to everyone at once), and I believe but lack a rigorous analysis that we get more privacy for a given amount of average delay if the transactions diffuse randomly in the network to hide where they originated from, at least if we assume the target doesn't send many transactions (and that an attacker isn't abusing multiple groups to get a faster effective send.)

E.g. imagine a user who originates from node X and an attacker that connects to almost all nodes in the network. If all sends happen at once, when the attacker sees a transaction happen from X first then he can have some confidence that transactions happened from there. If the send times are independent and the attacker sees a transaction from X first he doesn't learn that much from that transaction.

The issue becomes if the attacker connects to you 20 times and the times are independent than he gets effectively a 20th of the delay. Which is why I was suggesting leaving outbounds independent and putting all inbounds into a single group or a small number of groups.

src/net.cpp Outdated
{
std::pair<int, uint64_t> key{average_interval_seconds, net_group};
if (!inbound) {
return PoissonNextSend(now, INVENTORY_BROADCAST_INTERVAL >> 1);

This comment has been minimized.

@sipa

sipa May 21, 2018

Member

I think it's suboptimal to put knowledge of the actual scheduling policy in net; it belongs more in net_processing (the INVENTORY_BROADCAST_INTERVAL and NETWORK_GROUP_BUCKETS should move there as well, I think).

How about passing a boolean per_group in, or saying that when net_group = 0 there is no grouping to be done? Then you can keep the decision logic in net_processing, and keep this function generally usable.

@@ -3474,7 +3474,11 @@ bool PeerLogicValidation::SendMessages(CNode* pto, std::atomic<bool>& interruptM
if (pto->nNextInvSend < nNow) {
fSendTrickle = true;
// Use half the delay for outbound peers, as there is less privacy concern for them.
pto->nNextInvSend = PoissonNextSend(nNow, INVENTORY_BROADCAST_INTERVAL >> !pto->fInbound);
if (pto->fInbound) {

This comment has been minimized.

@sipa

sipa May 21, 2018

Member

I think this needs to be inverted.

This comment has been minimized.

@jamesob

jamesob Jul 13, 2018

Member

(comment addressed AFAICT)

src/net.cpp Outdated
int64_t CConnman::PoissonNextSendTo(int64_t now, int average_interval_seconds, uint64_t net_group_bucket)
{
LOCK(cs_net_bucket_next_sends);
std::map<uint64_t, int64_t>::iterator net_bucket_next_send = net_bucket_next_sends.find(net_group_bucket);

This comment has been minimized.

@sipa

sipa May 21, 2018

Member

Without the interval size in the map, the correctness of this function depends on there only being a single interval (which is the case now, so no problem). It would be good to document that in the net.h definition, or change it.

This comment has been minimized.

@jimpo

jimpo Jun 3, 2018

Contributor

Since that is the case, maybe average_interval_seconds should be removed as a parameter and just use the INVENTORY_BROADCAST_INTERVAL constant directly.

@sdaftuar

This comment has been minimized.

Copy link
Member

sdaftuar commented May 25, 2018

I've been looking at this paper "Deanonymization in the Bitcoin P2P Network" (https://papers.nips.cc/paper/6735-deanonymization-in-the-bitcoin-p2p-network.pdf). It's got some ideas for how to think about an adversary's strategies with respect to detecting the first node on a network to relay a new transaction.

It seems to me there are a couple different kinds of adversaries to consider -- (a) one that makes only inbound connections to listening nodes on the network, versus (b) one that manages to take over some percentage of listening nodes and thus controls x% of the outbound connections on the network. (And then these could be combined of course.)

For type (a) adversaries, there are multiple types of strategies that could be employed to deanonymize the first listening node to relay a transaction. Perhaps the simplest strategy is the "first spy" estimator described in that paper, where the adversary estimates the first node to relay a transaction to it as the source. Such a strategy seems to be very successful if the adversary connects to each node on the network multiple times, and this PR would mitigate that. However, based on simulations I've done, I think 8 buckets is too many. I don't have a sense for how easy it is for an adversary to actually connect from different network groups to achieve the 8 different timings -- so if we decide that is suitably hard, then maybe the point I'm raising is moot -- but if we assume that is achievable, then my simulations suggest a 40% success rate for a first-spy adversary.

The paper I mentioned above goes into some length discussing the success bounds of an attacker that is smarter than that and also uses knowledge of the network graph to estimate the source node. I am still working through simulations that demonstrate the ideas they suggest, but I think I have a rough maximum-likelihood estimator that appears to be -- very roughly -- 15-20% successful at estimating the source node based on the times at which the adversary sees announcements from each node in the network. This is assuming that the adversary has 1 inbound connection to each node on the network.

It seems to me that as a first pass, we might limit the number of buckets so that the effectiveness of a first-spy adversary is on par with that of a smarter adversary[1]. From my analysis so far, I think having 2 buckets for incoming peers would roughly achieve that, with the important caveats that (a) no one has sanity checked my sim results [2], and (b) I don't know yet how good the maximum-likelihood estimator could be if it were smart enough to use 2 connections to each peer.

If we weren't concerned about tradeoffs (ie bandwidth spikes or relay propagation delays), I'd say we could just make this all one bucket for inbound peers, to maximize source-privacy in our current transaction relay model.


[1] I think we could also go further and figure out some equilibrium with respect to an adversary controlling X% of the listening nodes/outbound connections on the network, but I'm not sure how to think about the relative costs of these kinds of adversarial approaches.

[2] If anyone is able to do their own simulations or estimations to corroborate my observations that would be fantastic.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented May 25, 2018

@sdaftuar Do you think there would be reasons to increase the number of groups when there are more connections? Clearly for small numbers of connections the concern about bandwidth spikes doesn't exist, but if a node has 1000 connections, perhaps it does.

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented May 25, 2018

@sdaftuar The suggestion I was leaning towards making but couldn't justify strongly was to multiply the interval for inbound peers by 2 and have 4 groups-- so that each of the 4 groups gets serviced 4x less often than a outbound peer . This would make an inbound oriented attacker get as much information from their inbound attack as being a attacker on a single outbound connection.

I would expect that 4 is still pretty bandwidth spike mitigating.

We also do want to have multiple groups so that we have a chance of 'routing around' a first spy monitor. Though perhaps the outbounds are sufficient for that.

I don't have a sense for how easy it is for an adversary to actually connect from different network groups to achieve the 8 different timings

Trivial, because our "network groups" are just /16s you can achieve the 8 groups just with a single EC2 account, picking different datacenters. Even if we got a bit smarter and put in some prefixes of major VPS providers to be treated as a single group (or one per provider), it's still not terrible hard to get on lots of networks... there are even commercial services that give you addresses in as many /8s as possible for the purpose of jamming the bittorrent DHT stuff for 'anti-piracy' reasons.

We should generally assume that network group limitations are a pretty modest roadbump. They're cheap to implement, so "why not".... but they don't to much.

also uses knowledge of the network graph to estimate the source node.

Thus future efforts on topology rotation, ... though there are tradeoffs there too. :(

@sipa

Do you think there would be reasons to increase the number of groups when there are more connections?

Well the attacker decides to connect to you, so naively doing that would be foolish since he could just get more information that way. Another way of looking at it is if you have 1000 connections presumably you have a lot more bandwidth and don't mind the spikes that much. If you don't like the spikes run fewer connections.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented May 28, 2018

Well the attacker decides to connect to you, so naively doing that would be foolish since he could just get more information that way.

Uh, of course. Ignore my suggestion.

The suggestion I was leaning towards making but couldn't justify strongly was to multiply the interval for inbound peers by 2 and have 4 groups-- so that each of the 4 groups gets serviced 4x less often than a outbound peer .

That seems reasonable, but some simulations - for example on the effect on network wide propagation speed would be useful.

@jimpo
Copy link
Contributor

jimpo left a comment

Concept ACK on limiting by inbound net group.

Not sure about the best selection of parameters without simulation, but this can always be tweaked later. I like @gmaxwell's suggestion of 4 buckets and increasing the broadcast interval on inbounds from 2x to 4x over outbound connections.

As for @sipa's suggestion of scaling buckets with the number of connections, it might be reasonable to scale with the configured max inbound connections, which not manipulable by an attacker.

@@ -35,6 +35,23 @@ static constexpr int64_t EXTRA_PEER_CHECK_INTERVAL = 45;
/** Minimum time an outbound-peer-eviction candidate must be connected for, in order to evict, in seconds */
static constexpr int64_t MINIMUM_CONNECT_TIME = 30;

/** Average delay between local address broadcasts in seconds. */

This comment has been minimized.

@jimpo

jimpo Jun 3, 2018

Contributor

I believe these can all go in net_processing.cpp instead of the header file.

This comment has been minimized.

@jimpo

jimpo Jul 5, 2018

Contributor

@naumenkogs This has not been addressed.

src/net.cpp Outdated
int64_t CConnman::PoissonNextSendTo(int64_t now, int average_interval_seconds, uint64_t net_group_bucket)
{
LOCK(cs_net_bucket_next_sends);
std::map<uint64_t, int64_t>::iterator net_bucket_next_send = net_bucket_next_sends.find(net_group_bucket);

This comment has been minimized.

@jimpo

jimpo Jun 3, 2018

Contributor

Since that is the case, maybe average_interval_seconds should be removed as a parameter and just use the INVENTORY_BROADCAST_INTERVAL constant directly.

@@ -3474,7 +3474,11 @@ bool PeerLogicValidation::SendMessages(CNode* pto, std::atomic<bool>& interruptM
if (pto->nNextInvSend < nNow) {
fSendTrickle = true;
// Use half the delay for outbound peers, as there is less privacy concern for them.
pto->nNextInvSend = PoissonNextSend(nNow, INVENTORY_BROADCAST_INTERVAL >> !pto->fInbound);
if (pto->fInbound) {

This comment has been minimized.

@jimpo

jimpo Jun 3, 2018

Contributor

For the sake of encapsulation, I think you can put more of this logic into int64_t CConnman::PoissonNextSendTo(int64_t now, CNode* node), including the check on fInbound and the half delay comment.

This comment has been minimized.

@sipa

sipa Jul 5, 2018

Member

@jimpo Does this comment still apply? CConnman::PoissonNextSendTo doesn't take a CNode* argument anymore, and I think it's better separation to keep the logic for determining the interval separate from the logic for computing the next send.

This comment has been minimized.

@jimpo

jimpo Jul 5, 2018

Contributor

I'd still prefer a separate method for these four lines CConnman::PoissonNextSendTo(int64_t now, CNode* node). Certainly having the separate PoissonNextSend which actually computes the interval is a good idea (which already exists), but CConnman::PoissonNextSendTo as defined currently just does the interval caching by netgroup. I find the interface weird and I don't understand the split between logic there and here.

That said, it's a pretty minor point and I don't feel too strongly.

@sdaftuar

This comment has been minimized.

Copy link
Member

sdaftuar commented Jun 6, 2018

I've been doing some more simulations on this recently. I still don't have good results for the graph-aware estimators that (in theory) should be pretty powerful. But I did want to share a couple other results on the first spy estimator:

First my previously reported simulations were more optimistic than I realized, because I was just modeling the effect of limiting the number of connections an adversary could make, without taking into account the delay in propagation across the network that occurs when we bin the inbound peers. So for instance I previously modeled how well an attacker would do if we limited them to 8 connections per peer, without also modeling the effect on the network due to ALL inbound peers being in those buckets as well.

Slowing down transaction relay across the network benefits the first-spy estimator (as it increases their window of success). So I fixed that deficiency in my latest simulation, and I observed that bucketing the inbound peers to 8 buckets -- and assuming that the adversary is in all 8 buckets for all nodes on the network -- appears to give a success rate to the attacker of ~54% (!).

I did my simulations on randomly generated graphs on 5000 nodes, where each node randomly picked 8 outbound peers. I assume the adversary is connected in every available inbound bucket. This uses the existing relay delays we have for inbound peers (2x the mean delay of our outbound peers). For each row in the table, I ran 2000 simulations.

Number of inbound buckets Success rate
1 19%
2 26%
3 34%
4 41%
5 46%
6 50%
7 53%
8 54%

I also simulated @gmaxwell's suggested strategy, of using 4 inbound buckets, but applying a bigger (4x instead of 2x) delay to inbound peers. That had a first-spy estimator success rate of 28%.

I'll update these results with some statistics about total propagation times soon...

@naumenkogs

This comment has been minimized.

Copy link

naumenkogs commented Jun 7, 2018

I've run a simulation to figure out how accurate might be a simple first-time spy under different conditions.

I've used a network of 1000 nodes only (random graphs, 8 outgoing connections), so you can consider the results as upper bound comparing to the real Bitcoin network. (cause more nodes will introduce noise)
500 simulations.

I've considered an attacker that connects to all of the nodes multiple times so that it takes over all of the inbound buckets.

R = inbound_connection_interval/outbound_connection_inverval

R = 2

Number of inbound buckets Success rate
1 13%
2 26%
4 50%
8 63%

R = 4

Number of inbound buckets Success rate
1 13%
2 21%
4 41%
8 56%

From these numbers, 2 buckets and R=4 seems optimal.

Both of mine and @sdaftuar simulations demonstrate that increasing R is useful.
However, this makes the assumption about outgoing connections more crucial.

I'm going to run more simulations soon.

@naumenkogs

This comment has been minimized.

Copy link

naumenkogs commented Jun 14, 2018

I've measured how R affects propagation delay. From my measurements, it looks like the difference is within 25-40% for R=2 comparing to R=4 (I think that's a lot, but probably acceptable for now), and the distribution of values (50% 75% 99% 100%) is similar for different number of buckets as well as different R.

Another point I've found is number of buckets itself does not affect propagation delay much — at least for a reasonable number (up to 8, see prior data), which is surprising for me. It may be because of the size of my simulation (only 1000 nodes).

Anyway, from my observations it seems like neither setting R=4 nor using a small number of buckets
affect propagation time very dramatically.

@sdaftuar may have more interesting results here.

@naumenkogs

This comment has been minimized.

Copy link

naumenkogs commented Jun 14, 2018

I was also wondering what if an attacker has control over 1 of 8 outgoing connections per each peer in the network.

In this case, modifying R does not make any difference: success rate is 37-40% for 1 or 2 bucket scenario (worst case for a spy) for R=2, 4, 8. Success rate for R=8 grows a bit slower with number of buckets comparing to R = 2, but not significantly (~5-10%).

Having less buckets helps in the same fashion as in the previous measurements, but 37% is very high as for the worst case for a spy.

If we consider that an attacker may take control over 1 outgoing per some fraction of nodes, then having less buckets still would be helpful to prevent first-spy.

@naumenkogs

This comment has been minimized.

Copy link

naumenkogs commented Jul 3, 2018

Here are my results on spikes.

Meta

The graph represents the distribution of times when a random public-IP node in the network sends INVs to its peers.

I used 1 tx/s for simulation (to get real bandwidth numbers below I believe we should multiply by 3 as this would be closer to the current tx rate, the bandwidth numbers presented in this message are already multiplied by 3), 5000 transactions for each run, the ratio of public IP nodes to private IP nodes in the network is 1 to 10 (which is kinda similar to the current one).

Analysis

Distributions

I guess what is important is the tail of the distribution – the probability of huge number of transactions being relayed within one interval of 5 seconds.
I won't say anything about buckets > 2, because my results show that anonymity properties are weak there. I'd say, once we have more nodes in the network and having connections to 100% of nodes will be harder to achieve, we may consider buckets > 2.

if we choose 1 bucket, r=2 is better because it is less spikey and the success rate is the same. If we are OK with eventual spikes of 18 KB/s, we should do 1 bucket and r=2.

If we compare 2-bucket results, the distribution is slightly better for r=2, while anonymity is better for r=4 (5% better). I think r=4 is better here (keep in mind though that it introduces 25-40% propagation delay according to my measurements). The eventual spikes here are 14 KB/s.
Obviously, r=3 would be something in the middle, I may do additional runs for that scenario, I'm not adding it here to avoid confusion.

Please share your thoughts.

Upd:
Raw data and more graphs (buckets > 2) are available here.

@laanwj laanwj added this to Blockers in High-priority for review Jul 5, 2018

@naumenkogs naumenkogs force-pushed the naumenkogs:delay_per_net_group branch from 30f4cd1 to 8060fbb Jul 5, 2018

@naumenkogs

This comment has been minimized.

Copy link

naumenkogs commented Jul 7, 2018

Here are my experimental results on propagation delay.

This was discussed on bitcoin irc with @gmaxwell and @sipa.

At first, the fact that number of buckets does not matter much was questioned. You can see precise results now. 1 bucket comparing to 8 is 20-25% worse for 50% coverage, and 5-10% worse for 100% coverage.

At the same time, making R=4 itself has an impact around 80% comparing to R=2. That's why I'm arguing for R=2 and 1 bucket (if we allow those spikes of 18 KB/s).

What is still surprising for me (and for some other people) is: with R=1 setting buckets to 1 is slightly less significant than doing the same with R=4 — see "max slowdown". We expected it to be more significant, because we expected that with R=1 nature of relay through incoming should matter more. Also be aware that absolute max slowdown is fairly low — up to 2 seconds.

I've briefly seen some graphs of @sdaftuar and it was something similar to what I've had — the same surprise.

@DrahtBot DrahtBot removed the Needs rebase label Jul 7, 2018

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented Jul 10, 2018

Anyone oppose changing this to 1 bucket? it would let you simplify the code. e.g. I think the map could be eliminated, etc... just the shared inbound peer state. I think the 18kb/s spikes are okay, we'll do much worse when a compact-block-cold peer requests a block from us...

@jamesob

This comment has been minimized.

Copy link
Member

jamesob commented Jul 13, 2018

utACK efa0092

Should the PR description be updated to clarify that we're going with a 1-bucket approach? I had to wade through the comment thread to verify that was the intent.

src/net.cpp Outdated
return nNow + (int64_t)(log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */) * average_interval_seconds * -1000000.0 + 0.5);
int64_t CConnman::PoissonNextSendInbound(int64_t now, int average_interval_seconds)
{
if (next_send_inv_to_incoming < now) {

This comment has been minimized.

@sipa

sipa Jul 13, 2018

Member

Given that you're reading from the atomic variable multiple times here, there is a risk that it was updated by two threads in parallel, resulting in two different timestamps being returned from this function - which would break the independence of sends between nodes.

This is a highly unlikely event, and it may even be impossible for now (due to limited threading in the network handing code). However, with the objective of doing it "by the book", I believe a correct implementation would be this:

int64_t previous = next_send_inv_to_incoming.load();
if (prevous < now) {
    int64_t next = PoissonNextSend(now, average_interval_seconds);
    do {
        if (next_send_inv_to_incoming.compare_exchange_weak(previous, next)) {
            // We succesfully updated the atomic next_send_inv_to_incoming
            return next;
        }
        // Another thread changed next_send_to_inv after we fetched previous,
        // but before we tried to update it. The values chosen by the other
        // thread is loaded into previous, so check if that value is >= now.
        if (previous >= now) return previous;
    }
}
return previous;

(this is not a blocker for my review)

@instagibbs
Copy link
Member

instagibbs left a comment

utACK 2a361eb

but I agree with @jamesob that the description needs updating. Took me a couple minutes to figure out the connection with what is quite simple code now.

src/net.h Outdated
@@ -434,6 +441,8 @@ class CConnman
* This takes the place of a feeler connection */
std::atomic_bool m_try_another_outbound_peer;

std::atomic<int64_t> next_send_inv_to_incoming;

This comment has been minimized.

@instagibbs

instagibbs Jul 13, 2018

Member

mu-nit: use m_ prefix for member variable

@naumenkogs naumenkogs changed the title Net: Random delays *per network group* to obfuscate transaction time Net: Bucketing INV delays (1 bucket) for incoming connections to hide tx time Jul 13, 2018

src/net.cpp Outdated

int64_t PoissonNextSend(int64_t now, int average_interval_seconds)
{
return now + (int64_t)(log1p(GetRand(1ULL << 48) * -0.0000000000000035527136788 /* -1/2^48 */) * average_interval_seconds * -1000000.0 + 0.5);

This comment has been minimized.

@Empact

Empact Jul 13, 2018

Member

nit: non-standard indentation here and elsewhere, clang-format will fix

/** Average delay between feefilter broadcasts in seconds. */
static const unsigned int AVG_FEEFILTER_BROADCAST_INTERVAL = 10 * 60;
/** Maximum feefilter broadcast delay after significant change. */
static const unsigned int MAX_FEEFILTER_CHANGE_DELAY = 5 * 60;

This comment has been minimized.

@Empact

Empact Jul 13, 2018

Member

Agree with @jimpo, these can move to net_processing.cpp

This comment has been minimized.

@sipa

sipa Jul 13, 2018

Member

Agree, and additionally they can become constexpr.

@@ -3497,7 +3497,11 @@ bool PeerLogicValidation::SendMessages(CNode* pto, std::atomic<bool>& interruptM
if (pto->nNextInvSend < nNow) {
fSendTrickle = true;
// Use half the delay for outbound peers, as there is less privacy concern for them.

This comment has been minimized.

@Empact

Empact Jul 13, 2018

Member

mu-nit: this comment would be slightly more relevant in the else block

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jul 13, 2018

utACK 2a361eb; just nits.

@jamesob

This comment has been minimized.

Copy link
Member

jamesob commented Jul 13, 2018

utACK 2a361eb

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented Jul 13, 2018

re-ACK

@sdaftuar

This comment has been minimized.

Copy link
Member

sdaftuar commented Jul 13, 2018

utACK, sorry I let this slip, I have a bunch of simulation results that I had been meaning to revisit and summarize here (and I haven't had a chance to really go through @naumenkogs's latest results) but my short thought is that 1 bucket for inbound peers is probably the right answer and at any rate this is a substantial improvement compared with current behavior.

ACK'ing now so that we can get this merged for 0.17 (and I will try to come back and post additional observations / simulation results here later for others to review)

@naumenkogs naumenkogs force-pushed the naumenkogs:delay_per_net_group branch from 2a361eb to 496f9dc Jul 14, 2018

src/net.cpp Outdated
{
if (m_next_send_inv_to_incoming < now) {
// There is a theoretical possibility of a race condition here
// For now it is impossible, because this variable may be updated by 1 thread only

This comment has been minimized.

@sipa

sipa Jul 14, 2018

Member

This sounds a bit contradictory: if there is a theoretical possibility, then it is not impossible.

Perhaps you can say "If this function were called from multiple threads simultaneously it would possible that both update the next send variable, and return a different result to their caller. This is not possible in practice as only the net processing thread invokes this function."?

@naumenkogs naumenkogs force-pushed the naumenkogs:delay_per_net_group branch from 496f9dc to d45b344 Jul 14, 2018

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Jul 14, 2018

re-utACK d45b344

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented Jul 15, 2018

re-ACK

@fanquake fanquake added this to the 0.17.0 milestone Jul 15, 2018

@jimpo

This comment has been minimized.

Copy link
Contributor

jimpo commented Jul 16, 2018

utACK d45b344

@Empact

This comment has been minimized.

Copy link
Member

Empact commented Jul 16, 2018

Looks like github is having some issues, but this has been merged in: f8d470e

@sipa sipa closed this Jul 16, 2018

mruddy pushed a commit to mruddy/bitcoin that referenced this pull request Jul 16, 2018

Merge bitcoin#13298: Net: Bucketing INV delays (1 bucket) for incomin…
…g connections to hide tx time

d45b344 Bucket for inbound when scheduling invs to hide tx time (Gleb)

Pull request description:

  It has been brought up to my attention that current random delays mechanism (originally intended to obfuscate transaction metadata) allows to easily estimate the time a transaction was received by a node.

  It may be done by connecting multiple observer nodes to the same node. Each of those nodes will generate its own schedule of delays. Combined metadata regarding those events from different sources allows an observer to estimate transaction time.

  After this patch a spy won't gain additional information by just creating multiple connections to a target.

Tree-SHA512: c71dae5ff350b614cb40a8e201fd0562d3e03e3e72a5099718cd451f0d84c66d5e52bbaf0d5b4b75137514c8efdedcc6ef4df90142b360153f04ad0721545ab1

sipa added a commit to sipa/bitcoin that referenced this pull request Jul 16, 2018

Merge bitcoin#13298: Net: Bucketing INV delays (1 bucket) for incomin…
…g connections to hide tx time

Pull request description:

  It has been brought up to my attention that current random delays mechanism (originally intended to obfuscate transaction metadata) allows to easily estimate the time a transaction was received by a node.

  It may be done by connecting multiple observer nodes to the same node. Each of those nodes will generate its own schedule of delays. Combined metadata regarding those events from different sources allows an observer to estimate transaction time.

  After this patch a spy won't gain additional information by just creating multiple connections to a target.

Tree-SHA512: c71dae5ff350b614cb40a8e201fd0562d3e03e3e72a5099718cd451f0d84c66d5e52bbaf0d5b4b75137514c8efdedcc6ef4df90142b360153f04ad0721545ab1

MarcoFalke added a commit to MarcoFalke/bitcoin that referenced this pull request Jul 16, 2018

Merge bitcoin#13679: Initialize m_next_send_inv_to_incoming
347b4ff Initialize m_next_send_inv_to_incoming (Pieter Wuille)

Pull request description:

  This fixes an uninitialized variable introduced in bitcoin#13298.

Tree-SHA512: 0c6fb164164141036fbbc955475650724bffdb3593c22946f55ac715fa162183bf9377a8390ee9d13f104be22bc417445e2c7fb3d4acf5e6236ac802e50f3e77

@sipa sipa removed this from Blockers in High-priority for review Jul 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment