swap: prevent debt cheques

[mortelli]

This PR:

• extends the processAndVerifyCheque function so that it returns an error when receiving a cheque that would put the node into debt.
  • to determine this, the tentative balance (after processing the cheque) is calculated, and if lower than 0 - ChequeDebtTolerance, an error is returned.
  • ChequeDebtTolerance is a new const set—for now—to roughly 20% of the payment threshold.
  • in principle, we should not accept any cheques that would put us into debt, but due to possible out-of-sync balances, we are allowing some negative resulting balances (as well as any positive ones).
  • this is tested by the new TestDebtCheques function.
• adds missing error handling in SWAP tests.
• removes testMsgBigPrice and TestPingPongChequeSimulation, as this test was considered out of place in the normal testing suite due to its stress-test nature.

closes #1931

[mortelli]

created issue #2078 to follow-up on rare errors.

[mortelli]

@ralph-pichler i have pushed a simplified version of the TestDebtCheques function per your suggestion

[ralph-pichler]

LGTM

[holisticode]

Can we be sure at this point that the actual balance has been updated from the previous iteration run? I think it does, but give it another thought please.

[mortelli]

thank you for raising this point, it's something i hadn't considered.

my instinct was that the code (from this side of the exchange) would not be able to send a message in a following iteration until it was done with the current one, and this would include updating the balance.

i added some prints to the updateBalance calls and the test for loop iterations, and wrote an execution of TestMultiChequeSimulation test to disk.

here are the initial results. you can see that sometimes there are multiple balance updates within a single iteration. however, these seem to be from the creditor side, so it does not matter that they occur one time per iteration—at least for the point you've raised.

this is the result obtained by filtering out the other peer's balance updates prints. it seems to confirm that every iteration does 1 balance update before moving on to the next one.

curiously, this might give us insight into the cause of #2078. it's very possible that by the time the creditor is allowed to receive the messages and account for them, the debitor is way ahead and the cheque it has sent is perceived as debt from the other side, for being too early. but i think it would have to process the cheque before doing the balance updates for this to explain the situation.

[Eknir]

Something occurred to me:
Why would we disconnect from a node that sends us too much money? Surely, receiving too much money can only be good, right? I understand the context of this PR and how, when properly accounted for, receiving a cheque that is worth too much opens up the possibility for an attack. However, I believe we can solve this situation in a more elegant manner than a disconnect or putting the peer on a blacklist.

Note the sentence:

and how, when properly accounted for, receiving a cheque that is worth too much opens up the possibility for an attack.

I propose to do the following when we receive a cheque that is worth too much: don't account for it properly :).

Assume Node A has a balance of -500 with node B, node B sends a cheque of 10000 to A. If we would properly account for this, node A would immediately respond with a cheque to B as 9500 is surely more than the payment threshold. But, if we instead just program the node to update the balance to 100 instead of 9500, there is no attack possible anymore. The amount 100 can have the same value as what ChequeDebtTolerance has now.

If nodes are programmed to behave in this way, the attack is not possible anymore. In a normal situation, the situation above will not happen.

[mortelli]

Something occurred to me:
Why would we disconnect from a node that sends us too much money? Surely, receiving too much money can only be good, right? I understand the context of this PR and how, when properly accounted for, receiving a cheque that is worth too much opens up the possibility for an attack. However, I believe we can solve this situation in a more elegant manner than a disconnect or putting the peer on a blacklist.

Note the sentence:

and how, when properly accounted for, receiving a cheque that is worth too much opens up the possibility for an attack.

I propose to do the following when we receive a cheque that is worth too much: don't account for it properly :).

Assume Node A has a balance of -500 with node B, node B sends a cheque of 10000 to A. If we would properly account for this, node A would immediately respond with a cheque to B as 9500 is surely more than the payment threshold. But, if we instead just program the node to update the balance to 100 instead of 9500, there is no attack possible anymore. The amount 100 can have the same value as what ChequeDebtTolerance has now.

If nodes are programmed to behave in this way, the attack is not possible anymore. In a normal situation, the situation above will not happen.

since we already have 2 approvals for this PR as-is, i suggest we open a different issue/PR for the solution you mention here.

i will merge the code in its current state as soon as i am able to, i.e. when master is fixed so that the CI passes.