incentives requirements for protocol package

[ralph-pichler]

For incentives to properly enforce its disconnect penalty certain features that are not currently available in the protocols package are required.

• Both Peer.send and Peer.handleIncoming call the accounting hook which then calls and returns the return value of the swap accounting Add function (which is not aware if this is a send or receive). However the error returned from the accounting hook is handled differently in the two cases. In the case of Peer.send it is returned to the caller (where it is the simply logged and ignored in retrieval) and in the case of Peer.handleIncoming it instantly terminates the protocol. That means returning an error is not a reliable way of terminating a connection. A possible alternative would be to manually call Peer.drop (which has the advantage of dropping all protocols at once) but that would mean the accounting hook return value is pointless. Ideally the return value of the hook would have the same effect regardless of wether it was send or handleIncoming.

• Even if Add can successfully disconnect the peer, it is pointless if the peer can just keep reconnecting. (While technically it would then be dropped again on the first accounted message, there seems to be nothing stopping it from connecting to a protocol and simply never sending an accounted message while still using up a peer connection). Ideally there would be a blacklist that prevents overdraft peers from connecting to all (or at least all accounted) protocols.

• Currently nothing stops a peer from connecting on retrieval protocol but not connecting on swap protocol. This has similar problems to the previous point (we saw this happen and blocking retrieval requests at the incentives workshop at devcon, see enforce a swap-only network #1917). While with proper disconnects from the accounting hook this would be detected after the first accounted message, this might lead to a race condition where e.g. the retrieval protocols connects first and sends the first RetrieveRequest before the swap handshake finished (e.g. because the HandshakeMsg got delayed or the connection to the ethereum backend was slow), thus leading to a disconnect. Ideally there would be a way to ensure that accounted protocols only start after the swap protocol has finished starting.

Nice to haves:

• Based on a discussion during a research call (Review error and exception handling #1601 (comment)) there was also the idea that sometimes we want to block sending and receiving message from the accounting hook without disconnecting (e.g. debt-increasing messages if we know we are in debt and above the payment threshold but have no money left in the chequebook).

Note: This requirements were determined prior to the new swap pricing ideas. With retrieval being more aware of pricing in the new model there might be bigger changes necessary to the design of swap.

[holisticode]

fyi @janos

[mortelli]

Additional thoughts:

• if a blacklist is implemented, we should probably think of a way of getting peers out of it.
• it's still not clear to me whether dropping a peer and dropping messages to/from it (while still keeping it as a peer) are—in practice—any different, or if they have any advantage over each other.

[janos]

As we discussed in the round table, one of the solution could be to return specific errors (or error types) in handlers and handle peer drop or blacklist in protocols package. We can implement that when we change protocols package message handling part to be asynchronous.

[holisticode]

Additional thoughts:

• if a blacklist is implemented, we should probably think of a way of getting peers out of it.

👍

• it's still not clear to me whether dropping a peer and dropping messages to/from it (while still keeping it as a peer) are—in practice—any different, or if they have any advantage over each other.

Well if we withhold messages to it - keeping it still as a peer - there is the option that other protocols can still use the peer. This use case has often been mentioned, but there is nowhere a clear requirement for it (as far as I know).