swarm icon indicating copy to clipboard operation
swarm copied to clipboard

swap: prevent bad cheques from being sent

Open mortelli opened this issue 5 years ago • 6 comments

WIP

This PR modifies the SWAP Add function so that chequebook balance is inspected before writing a cheque.

If the cheque's increase of cumulative payout is larger than the chequebook's available balance, the cheque will not be set nor sent and an error will be returned.

closes #1885

Current Issues

  • due to new deadlock, peer lock calls in the Cheques function had to be (temporarily) commented out.
    • Add activates a peer lock when called, and once AvailableBalance was called during accounting, it attempts to call the Cheques function. As an RPC, Cheques attempts to hold this peer lock as well, which causes a deadlock.
    • ~i'm not convinced with the idea of using RPCs internally, i.e. for "regular" SWAP logic. i'd prefer (at least) wrapper functions for RPCs which are not used anywhere else.~
    • ~this also may bring back the discussion of whether (or not) to have specific functions for sent and received cheques (see points raised by @acud on #1863), be it for a specific peer or for all of them.~
    • ~this now also has to be updated to include pending cheques instead, when they exist.~
  • ~TestMultiChequeSimulation is failing with its original maxCheques value of 10, since it seems the cumulative payouts amount to more than the chequebook's available balance.~

mortelli avatar Nov 01 '19 20:11 mortelli

Adding the help needed tag in an attempt to solve current issues and move ahead with development.

mortelli avatar Nov 14 '19 18:11 mortelli

Slightly modified from the discussion here we want to use this formula for determining the available balance for a peer: s.contract.liquidBalanceFor(peer) - (SUM(forall peers p, p.getLastCheque*().CumulativeAmount) - s.contract.totalPaidOut()).

  • The totalPaidOut call is only available in the new ERC20 chequebook.
  • In the sum the pending cheque must be used if it exists
  • To avoid having to load the last sent / pending cheque for every peer we can also separately store the sum of the CumulativeAmounts or compute it at startup. As this value only increases when we create a pending cheque we just have to update the cached value there.

Rationale:

  • SUM(forall peers p, p.getLastCheque*().CumulativeAmount) - s.contract.totalPaidOut() is the total amount of still uncashed cheques. s.contract.liquidBalanceFor(peer) is the onchain balance available for a peer (including hard deposits). By reducing it by the first value we get the amount of the liquid balance that is not yet promised by another cheque.
  • This formula has the advantage that it only requires 2 eth_Call operations to get the latest value.

ralph-pichler avatar Nov 19 '19 18:11 ralph-pichler

i'm not convinced with the idea of using RPCs internally, i.e. for "regular" SWAP logic. i'd prefer (at least) wrapper functions for RPCs which are not used anywhere else.

we've discussed with the rest of the incentives team on 19/11, and came to the conclusion that we should use wrappers to expose business logic methods as RPC functions.

mortelli avatar Nov 20 '19 12:11 mortelli

due to new deadlock, peer lock calls in the Cheques function had to be (temporarily) commented out.

Add activates a peer lock when called, and once AvailableBalance was called during accounting, it attempts to call the Cheques function. As an RPC, Cheques attempts to hold this peer lock as well, which causes a deadlock.

also discussed on 19/11: we're going to wait for the new implementation of AvailableBalance before revisiting this issue.

mortelli avatar Nov 20 '19 12:11 mortelli

@mortelli , what is the status here?

Eknir avatar Feb 06 '20 11:02 Eknir

@mortelli , what is the status here?

still in progress, but not actively being worked on right now

mortelli avatar Feb 25 '20 14:02 mortelli