Opened 10 months ago

Last modified 9 months ago

#2245 new defect

Established routers counted as "new" in OBEP throttle

Reported by: zab Owned by:
Priority: minor Milestone: undecided
Component: router/general Version: 0.9.34
Keywords: Cc:
Parent Tickets:

Description

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.

Subtickets (add)

Attachments (1)

dropAtOBEP.png (700.7 KB) - added by zab 9 months ago.
dropAtOBEP vanilla vs patched

Download all attachments as: .zip

Change History (5)

comment:1 Changed 10 months ago by zab

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 10 months 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 9 months ago by zab

dropAtOBEP vanilla vs patched

comment:3 Changed 9 months ago by zab

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

Note: See TracTickets for help on using tickets.