interledgerjs / ilp-connector

Reference implementation of an Interledger connector.
Other
133 stars 59 forks source link

consider broadcasting routes when they change, not every 30 seconds #343

Open michielbdejong opened 7 years ago

michielbdejong commented 7 years ago

this might be an optimization (more long-term)

momerath42 commented 7 years ago

This is already what we do.

emschwartz commented 7 years ago

That seems like it would create way too much chattiness, and possibly never-ending circles of updates, no?

If you just sent out broadcasts on a fixed schedule some of the new information you get wouldn't make it into each cycle but then you'd only have 2 messages for each bilateral connection per time period.

momerath42 commented 7 years ago

Here's what we have now:

Events initiating a route-broadcast message:

  1. startup (send locally-configured routes)
  2. at an interval less than the hold-down-time the connector has given to its peers for previously broadcast routes (if there are no new destinations, this is only a heartbeat message)
  3. upon adding a route, and noting that it represents a new destination, or a different next-hop than it has previously had a route for (a topological change)

Those messages have contents in new_routes only when a given peer has not previously been sent those routes (with newness being defined as in 3 above).

The unreachable_through_me field is populated when a broken link is detected, for which there is no alternative. In other words, only when a previously advertised destination becomes unreachable. The message is sent on the usual broadcast schedule, and not immediately, so as to minimize 'flapping'. This is determined when a route is removed, either because a heartbeat wasn't received before the hold-down-time has elapsed, or because broadcasting to a peer fails.

michielbdejong commented 7 years ago

wow, this is a super-useful explanation, I'm going to add that to the docs, or is it already described in these terms somewhere?

upon adding a route, and noting that it represents a new destination, or a different next-hop

shouldn't that be: "if a destination is added/removed, or if its liquidity curve changes"? I think the next-hop is only relevant for determining split-horizon condition, which iiuc we're not currently doing?

and then I would maybe limit liquidity curve changes for the same route from triggering two consecutive updates within one hold-down-time, especially if they are small improvements.

momerath42 commented 7 years ago

liquidity curve changes don't trigger new_routes additions. For the logic surrounding whether a route is considered to be new and trigger a rebroadcast, see routing-tables.js addRoute and the handling of its return value by message-router.js receiveRoutes. I'm not happy with the present state of things.