Don't let swarm cause ChequeBounced events

[Eknir]

A Swarm never trigger a ChequeBounced event on the chequebook smart-contract side. Currently, a node will happily boot up and allow you to send cheques with an unfunded chequebook. This will cause errors on the node who received your cheques and tries to cash them out.

We can solve this issue by shutting down the node when the availableBalance is less or equal than the paymentThreshold.

dependent upon #1884

[mortelli]

A Swarm never trigger a ChequeBounced event on the chequebook smart-contract side.

What do you mean by this?

Currently, a node will happily boot up and allow you to request cheques with an unfunded chequebook.

As far as I know, we don't request cheques anymore.

This will cause errors on the node who received your cheques and tries to cash them out.

We can solve this issue by shutting down the node when the availableBalance is less or equal than the paymentThreshold.

We should for sure look into doing something about these errors. I am not fully convinced that shutting the node down is the answer here, though. Is it too harsh?

[Eknir]

What do you mean by this?

If the chequebook is presented a cheque which he cannot pay out (insufficient liquid balance), a ChequeBounced event is emitted. In the current implementation, this does not have any effect, but the idea is that all peers connected to this peer will immediately disconnect from this peer when they hear about this event, as the peer has proven not to be able to pay out his outstanding cheques (which is very bad).

As far as I know, we don't request cheques anymore.

Error on my side. I meant send cheques instead of request cheques. Edited the original text.

Is it too harsh?

It could be too harsh. We want to prevent the Add function in swap.go to be called with a negative amount, which means that the node operator won't be able to request any files from Swarm and that he cannot only answer a chequeDeliveryRequest when he has the content locally available.
I would say that for the sake of this issue, we can shut down the node, making a subsequent issue to handle it in a more elagant way.

[mortelli]

If the chequebook is presented a cheque which he cannot pay out (insufficient liquid balance), a ChequeBounced event is emitted. In the current implementation, this does not have any effect, but the idea is that all peers connected to this peer will immediately disconnect from this peer when they hear about this event, as the peer has proven not to be able to pay out his outstanding cheques (which is very bad).

I think that doing something with a ChequeBounced event is one thing, and this issue is another.

This issue to me sounds like preventing a node from emitting bad cheques. A counterpart to this would be disconnecting from nodes that are known to write bad cheques.

It could be too harsh. We want to prevent the Add function in swap.go to be called with a negative amount, which means that the node operator won't be able to request any files from Swarm and that he cannot only answer a chequeDeliveryRequest when he has the content locally available.
I would say that for the sake of this issue, we can shut down the node, making a subsequent issue to handle it in a more elegant way.

Calling Add with a negative amount is not inherently a bad thing; no bad cheque needs to be written for this to happen. I interpret your suggestion as: we shouldn't let a node receive chunks if said node is known not to be able to write cheques.

As you very rightly say, we could allow traffic in one direction. I would invite more opinions on this matter: whether to leave this for a future issue or implement it now.

---

Apart from this, I have 2 questions:

1. wouldn't peers that write bad checks be indirectly dealt with through the disconnect threshold anyway? wouldn't this make it an optimization, and less of a priority?
2. do we really want to shut down (or impair) a node when this condition is met? Couldn't funds be added later to allow cheques to be cashed out in the future?

[Eknir]

This issue to me sounds like preventing a node from emitting bad cheques. A counterpart to this would be disconnecting from nodes that are known to write bad cheques.

This issue is indeed about preventing a node from writing bad cheques. Created an issue for taking action when a node notices that a peer wrote a bad cheque. #1895

I interpret your suggestion as: we shouldn't let a node receive chunks if said node is known not to be able to write cheques.

Yes. Not allowing to call add with a negative number is indeed not correct. Calling Add with a negative number is a consequence of something, and this consequence we should not allow: sending chunk requests.

wouldn't peers that write bad checks be indirectly dealt with through the disconnect threshold anyway? wouldn't this make it an optimization, and less of a priority?

No, even bad cheques will be accepted as a valid cheque by the counterparts. It is only at the moment that a peer tries to cash a cheque, that major disruptions can occur, as cashing a cheque possibly fails. the error handling discussion proposes to deal with a failure of cashing a cheque. This issue is about preventing this error from ever happening. I think this issue has a high priority.

do we really want to shut down (or impair) a node when this condition is met? Couldn't funds be added later to allow cheques to be cashed out in the future?

We should at least never allow a node to cross the payment threshold (as no cheques can be send).
Ideally, we have some sort of no-balance mode in Swap, which means that the node will allow to incur costs with its peers up to the payment threshold, after which only actions that don't incur costs are allowed with this peer. This mode is useful for this issue, but also for no-eth entry into Swarm.

I am putting this on the agenda for the research office hours, but I would propose that for now, we just stop SWAP.

[Eknir]

@mortelli took upon the task of finishing this issue by means of #1925. This issue is still in draft. I asked @mortelli in the PR description about the status and we will try to proceed.