Opened 3 years ago

Closed 4 months ago

#2245 closed defect (fixed)

Established routers counted as "new" in OBEP throttle

Reported by: Zlatin Balevsky Owned by: zzz
Priority: minor Milestone: 0.9.50
Component: router/general Version: 0.9.34
Keywords: Cc:
Parent Tickets: Sensitive: no


The new connection OBEP throttle increments _newRouterCount even if the comm system says we're connected to that router. As result, the throttle ends up being much tighter than intended.


Attachments (1)

dropAtOBEP.png (700.7 KB) - added by Zlatin Balevsky 3 years ago.
dropAtOBEP vanilla vs patched

Download all attachments as: .zip

Change History (8)

comment:1 Changed 3 years ago by Zlatin Balevsky

Proposed change to the logic: http://zerobin.i2p/?f2a07e2fad65b8c0#jCAppgCn8g+j/OKsP6QCbSjXFGZoW/QTEgIUKnVjmOU=

Always allow routers that are already connected to distribute the message.

comment:2 Changed 3 years ago by zzz

You may wish to propose a different algorithm, but the current code does match my original intent - that it counts all targets, whether connected or not, and once over the limit, starts dropping messages to targets that aren't connected. The javadoc says "limit of new routers" which iirc means "new/unique for this OBEP", not "unconnected for this router".

Also, at line 30, the comment is that the limit of 60 is probably too high and will be reduced later.

The current code does "Always allow routers that are already connected to distribute the message." Your patch changes the way we count.

Carrying over some discussion from IRC -

  • I don't believe these limits should vary based on router capacity, as these are per-OBEP limits, independent of total capacity.
  • I suspect the sources of these rogue tunnels are DHT apps excessively bursting, or hugely busy servers with too few tunnels configured, or routers that are doing netdb searches or refreshes or tunnel builds in a burst manner.
  • This throttle was put in to fix real problems with a single tunnel driving routers to conn limits and that's the protection it provides. The fact that the throttle is actually throttling - even frequently - is not, in itself, evidence that it's too "tight". or tighter than intended.

Changed 3 years ago by Zlatin Balevsky

Attachment: dropAtOBEP.png added

dropAtOBEP vanilla vs patched

comment:3 Changed 3 years ago by Zlatin Balevsky

Attached is a graph of "tunnel.dropAtOBEP" rate with a vanilla router vs one with the patch. I was going to let it run for the same amount of time but after just 3 hours it's obvious the difference is drastic.

So, I propose we adopt this method of counting. It does not affect the rate at which new connections are established; I can provide "router.activePeers" if desired, but it looks about the same.

comment:4 Changed 3 years ago by zzz

Sure, if you count differently, you will drop less. But if we do count your way, I think the 60 limit is way (way) way too high (and as I said above it's probably too high now anyway). My point is, dropping isn't necessarily bad, and less dropping isn't necessarily good.

The theory behind the current code: A reasonable tunnel won't send messages to more than 60 new routers every 30 seconds. Above that limit, if not already connected, a message to a new and unconnected router will be dropped. Note that the _toRouters Set is never cleared… so once you've sent something to that router, any other messages to that router won't be dropped, for the life of the tunnel.

Allowing 60 new conns per 30 seconds from a single tunnel will drive just about any router to conn limits and I think could be the basis for an attack? Will have to think about it.

Perhaps we need to step back and consider the "reasonable tunnel" assumption, and perhaps look for sources of what's currently classified as unreasonable. I listed some ideas in comment 2. Maybe i2psnark, maybe i2pd, maybe biglybt…

comment:5 Changed 4 months ago by jogger

Sensitive: unset

I cannot access the original patch, but implemented the logic below. I found no noticeable increase in connection numbers, probably because we time out inactive cons. Current code implements random drop depending on order of message arrival, as first 60 targets are always permitted. It also accesses the hash set and the clock more often than needed. With the code below MAX_ROUTERS_PER_PERIOD could be set much lower then if such a throttle is desired.

            if (!_toRouters.add(target) || _context.commSystem().isEstablished(target) || ++_newRouterCount <= MAX_ROUTERS_PER_PERIOD)
                return false;
            long now = _context.clock().now();
            if (_newRouterTime < now - NEW_ROUTER_PERIOD) {
                // latest guy is outside previous period
                _newRouterCount = 1;
                _newRouterTime = now;
                return false;
            // rarely get here at current limits
        return true;

comment:6 Changed 4 months ago by zzz

Milestone: undecided0.9.50
Owner: set to zzz
Status: newaccepted

Not clear why we didn't follow through on this 3 years ago.

I asked zab if he still had the original patch but he doesn't.

Your patch looks reasonable, I have it queued up for testing.

comment:7 Changed 4 months ago by zzz

Resolution: fixed
Status: acceptedclosed
Note: See TracTickets for help on using tickets.