Skip to content
This repository has been archived by the owner on Aug 2, 2021. It is now read-only.

swap: prevent debt cheques #1983

Merged
merged 65 commits into from
Feb 6, 2020
Merged
Show file tree
Hide file tree
Changes from 54 commits
Commits
Show all changes
65 commits
Select commit Hold shift + click to select a range
671a30d
swap: add DefaultChequeDebtTolerance const
mortelli Nov 26, 2019
2723804
swap: add code to prevent accepting cheques which would put the node …
mortelli Nov 26, 2019
75cfe23
swap: rename DefaultChequeDebtTolerance const to ChequeDebtTolerance
mortelli Nov 26, 2019
4b85c4d
swap: improve bad cheques prevention comments
mortelli Nov 26, 2019
01b42ab
Merge remote-tracking branch 'origin/master' into swap-prevent-debt-c…
mortelli Nov 28, 2019
8df2245
Merge remote-tracking branch 'origin/master' into swap-prevent-debt-c…
mortelli Dec 4, 2019
d69ced8
Merge remote-tracking branch 'origin/master' into swap-prevent-debt-c…
mortelli Dec 5, 2019
6bc0fe5
swap: duplicate ChequeDebtTolerance
mortelli Dec 6, 2019
6044ec6
Merge remote-tracking branch 'origin/master' into swap-prevent-debt-c…
mortelli Dec 11, 2019
d0e8146
Merge remote-tracking branch 'origin/master' into swap-prevent-debt-c…
mortelli Dec 23, 2019
619129f
Merge remote-tracking branch 'origin/master' into swap-prevent-debt-c…
mortelli Jan 9, 2020
8ee2e2c
swap: simplify ChequeDebtTolerance calculation
mortelli Jan 9, 2020
5f2b9f5
swap: fix bug in processAndVerifyCheque
mortelli Jan 9, 2020
0aa47da
swap: add first iteration for TestDebtChequeTolerance
mortelli Jan 9, 2020
b271f2d
swap: iterate TestDebtChequeTolerance test
mortelli Jan 10, 2020
ab755d9
swap: TestDebtChequeTolerance
mortelli Jan 10, 2020
4a6c3a3
swap: add TestDebtCheques func
mortelli Jan 10, 2020
f02a9f2
swap: iterate TestDebtCheques
mortelli Jan 10, 2020
03484ad
swap: iterate TestDebtCheques
mortelli Jan 10, 2020
daa620a
swap: iterate TestDebtCheques function
mortelli Jan 10, 2020
eb7d34a
swap: iterate TestDebtCheques function
mortelli Jan 10, 2020
068b58c
swap: iterate TestDebtCheques function
mortelli Jan 10, 2020
9ba613d
swap: add missing error handling in swap tests
mortelli Jan 10, 2020
b1a0cfc
Merge remote-tracking branch 'origin/master' into swap-prevent-debt-c…
mortelli Jan 10, 2020
ba65ef7
swap: simplify error handling in swap tests
mortelli Jan 10, 2020
d558c16
swap: small refactor in tests
mortelli Jan 13, 2020
1ceb4f2
Merge remote-tracking branch 'origin/master' into swap-prevent-debt-c…
mortelli Jan 13, 2020
ba551bc
swap: refactor TestDebtCheques
mortelli Jan 13, 2020
03d601b
swap: iterate TestDebtCheques function
mortelli Jan 13, 2020
c7ceba3
swap: add missing error catch for TestTriggerPaymentThreshold
mortelli Jan 13, 2020
f044532
swap: fix typo in TestDebtCheques
mortelli Jan 13, 2020
1f386ae
Merge remote-tracking branch 'origin/master' into swap-prevent-debt-c…
mortelli Jan 15, 2020
e17b503
swap: fix bug in processAndVerifyCheque return
mortelli Jan 16, 2020
22b4dd3
swap: fix formatting in TestDebtCheques
mortelli Jan 16, 2020
7706df9
swap: add testMsgSmallPrice type
mortelli Jan 16, 2020
4d33e9e
swap: remove TestPingPongChequeSimulation
mortelli Jan 16, 2020
de01e2d
swap: iterate TestMultiChequeSimulation
mortelli Jan 16, 2020
5869533
swap: iterate TestMultiChequeSimulation
mortelli Jan 16, 2020
78561f2
swap: iterate TestMultiChequeSimulation
mortelli Jan 16, 2020
b449607
swap: iterate TestMultiChequeSimulation
mortelli Jan 16, 2020
1e815e8
swap: iterate TestMultiChequeSimulation
mortelli Jan 16, 2020
f34200a
swap: iterate TestMultiChequeSimulation
mortelli Jan 17, 2020
585e77d
swap: iterate TestMultiChequeSimulation, remove testMsgBigPrice
mortelli Jan 17, 2020
5e53773
swap: fix wrong ChequeDebtTolerance computation
mortelli Jan 17, 2020
fd99c9c
swap: call Equal for cheques comparison in TestDebtCheques
mortelli Jan 17, 2020
9102171
swap: iterate TestDebtCheques
mortelli Jan 17, 2020
1ff452c
swap: iterate TestDebtCheques
mortelli Jan 17, 2020
f4757e2
swap: iterate TestDebtCheques
mortelli Jan 17, 2020
5010458
swap: add debug log to TestMultiChequeSimulation
mortelli Jan 20, 2020
39f3aae
swap: add check for all messages received in TestMultiChequeSimulatio…
mortelli Jan 20, 2020
5b6f493
swap: remove debug print in TestMultiChequeSimulation
mortelli Jan 20, 2020
6ca077b
swap: small refactor to TestMultiChequeSimulation
mortelli Jan 20, 2020
8e8df8a
Revert "swap: small refactor to TestMultiChequeSimulation"
mortelli Jan 20, 2020
3faf60a
swap: simplify TestDebtCheques function
mortelli Jan 21, 2020
63229eb
Merge remote-tracking branch 'origin/master' into swap-prevent-debt-c…
mortelli Jan 21, 2020
7d1ce9c
swap: remove extra context in TestMultiChequeSimulation
mortelli Jan 22, 2020
1d885fc
swap: remove randomness in TestMultiChequeSimulation
mortelli Jan 22, 2020
77a153b
swap: fix wrong error message in sendCheque function
mortelli Jan 22, 2020
7fa9e47
Merge remote-tracking branch 'origin/master' into swap-prevent-debt-c…
mortelli Jan 28, 2020
f1a46a0
Merge remote-tracking branch 'origin/master' into swap-prevent-debt-c…
mortelli Jan 29, 2020
d2402bb
Merge remote-tracking branch 'origin/master' into swap-prevent-debt-c…
mortelli Jan 30, 2020
594d900
Merge remote-tracking branch 'origin/master' into swap-prevent-debt-c…
mortelli Jan 30, 2020
415e7f6
swap: fix wrong types in tests
mortelli Jan 30, 2020
8888cfc
Merge remote-tracking branch 'origin/master' into swap-prevent-debt-c…
mortelli Feb 5, 2020
3117c81
Merge remote-tracking branch 'origin/master' into swap-prevent-debt-c…
mortelli Feb 6, 2020
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions swap/config.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,9 @@ const (
// DefaultPaymentThreshold is set to be equivalent to requesting and serving 10mb of data (2441 chunks (4096 bytes) = 10 mb, 10^7 bytes = 10 mb)
DefaultPaymentThreshold = 2441*RetrieveRequestPrice + (10^7)*ChunkDeliveryPrice // 4096 * 2441 = 10 mb,
DefaultDisconnectThreshold = 20 * DefaultPaymentThreshold
// ChequeDebtTolerance is the lowest resulting balance a node is willing to accept when receiving a cheque
// the value is meant to be used below 0, as positive resulting balances should always be accepted when receiving cheques
ChequeDebtTolerance = DefaultPaymentThreshold * 20 / 100 // roughly 20% of the payment threshold
// DefaultDepositAmount is the default amount to send to the contract when initially deploying
// NOTE: deliberate value for now; needs experimentation
DefaultDepositAmount = 0
Expand Down
8 changes: 6 additions & 2 deletions swap/protocol_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -359,7 +359,9 @@ func TestTriggerPaymentThreshold(t *testing.T) {
// set the balance to manually be at PaymentThreshold
overDraft := 42
expectedAmount := uint64(overDraft) + DefaultPaymentThreshold
creditor.setBalance(-int64(DefaultPaymentThreshold))
if err = creditor.setBalance(-int64(DefaultPaymentThreshold)); err != nil {
t.Fatal(err)
}

// we expect a cheque at the end of the test, but not yet
if creditor.getLastSentCheque() != nil {
Expand Down Expand Up @@ -494,7 +496,9 @@ func TestTriggerDisconnectThreshold(t *testing.T) {
overDraft := 42
expectedBalance := int64(DefaultDisconnectThreshold)
// we don't expect any change after the test
debitor.setBalance(expectedBalance)
if err = debitor.setBalance(expectedBalance); err != nil {
t.Fatal(err)
}
// we also don't expect any cheques yet
if debitor.getPendingCheque() != nil {
t.Fatalf("Expected no cheques yet, but there is %v", debitor.getPendingCheque())
Expand Down
220 changes: 68 additions & 152 deletions swap/simulations_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import (
"fmt"
"io/ioutil"
"math/big"
"math/rand"
"os"
"strings"
"sync"
Expand Down Expand Up @@ -74,7 +75,7 @@ type swapSimulationParams struct {
// define test message types
type testMsgBySender struct{}
type testMsgByReceiver struct{}
type testMsgBigPrice struct{}
type testMsgSmallPrice struct{}

// create a test Spec; every node has its Spec and its accounting Hook
func newTestSpec() *protocols.Spec {
Expand All @@ -85,7 +86,7 @@ func newTestSpec() *protocols.Spec {
Messages: []interface{}{
testMsgBySender{},
testMsgByReceiver{},
testMsgBigPrice{},
testMsgSmallPrice{},
},
}
}
Expand All @@ -106,9 +107,9 @@ func (m *testMsgByReceiver) Price() *protocols.Price {
}
}

func (m *testMsgBigPrice) Price() *protocols.Price {
func (m *testMsgSmallPrice) Price() *protocols.Price {
return &protocols.Price{
Value: DefaultPaymentThreshold + 1,
Value: DefaultPaymentThreshold / 100, // ensures that the message won't put nodes into debt
PerByte: false,
Payer: protocols.Sender,
}
Expand Down Expand Up @@ -250,135 +251,6 @@ func newSharedBackendSwaps(t *testing.T, nodeCount int) (*swapSimulationParams,
return params, nil
}

// TestPingPongChequeSimulation just launches two nodes and sends each other
// messages which immediately crosses the PaymentThreshold and triggers cheques
// to each other. Checks that accounting and cheque handling works across multiple
// cheques and also if cheques are mutually sent
func TestPingPongChequeSimulation(t *testing.T) {
nodeCount := 2
// create the shared backend and params
params, err := newSharedBackendSwaps(t, nodeCount)
if err != nil {
t.Fatal(err)
}
// cleanup backend
defer params.backend.Close()

// setup the wait for mined transaction function for testing
cleanup := setupContractTest()
defer cleanup()

params.backend.cashDone = make(chan struct{}, 1)
defer close(params.backend.cashDone)

// initialize the simulation
sim := simulation.NewBzzInProc(newSimServiceMap(params), false)
defer sim.Close()

log.Info("Initializing")

// we are going to use the metrics system to sync the test
// we are only going to continue with the next iteration after the message
// has been received on the other side
metricsReg := metrics.AccountingRegistry
// testMsgBigPrice is paid by the sender, so the credit counter will only be
// increased when receiving the message, which is what we want for this test
cter := metricsReg.Get("account.msg.credit")
counter := cter.(metrics.Counter)
counter.Clear()
var lastCount int64
expectedPayout1, expectedPayout2 := DefaultPaymentThreshold+1, DefaultPaymentThreshold+1

_, err = sim.AddNodesAndConnectFull(nodeCount)
if err != nil {
t.Fatal(err)
}

p1 := sim.UpNodeIDs()[0]
p2 := sim.UpNodeIDs()[1]
ts1 := sim.Service("swap", p1).(*testService)
ts2 := sim.Service("swap", p2).(*testService)

var ts1Len, ts2Len, ts1sLen, ts2sLen int
timeout := time.After(10 * time.Second)

for {
// let's always be nice and allow a time out to be catched
select {
case <-timeout:
t.Fatal("Timed out waiting for all swap peer connections to be established")
default:
}
// the node has all other peers in its peer list
ts1.swap.peersLock.Lock()
ts1Len = len(ts1.swap.peers)
ts1sLen = len(ts1.peers)
ts1.swap.peersLock.Unlock()

ts2.swap.peersLock.Lock()
ts2Len = len(ts2.swap.peers)
ts2sLen = len(ts2.peers)
ts2.swap.peersLock.Unlock()

if ts1Len == 1 && ts2Len == 1 && ts1sLen == 1 && ts2sLen == 1 {
break
}
// don't overheat the CPU...
time.Sleep(5 * time.Millisecond)
}

maxCheques := 42

ts1.lock.Lock()
p2Peer := ts1.peers[p2]
ts1.lock.Unlock()

ts2.lock.Lock()
p1Peer := ts2.peers[p1]
ts2.lock.Unlock()

for i := 0; i < maxCheques; i++ {
if i%2 == 0 {
if err := p2Peer.Send(context.Background(), &testMsgBigPrice{}); err != nil {
t.Fatal(err)
}
if err := waitForChequeProcessed(t, params.backend, counter, lastCount, ts1.swap.peers[p2], expectedPayout1); err != nil {
t.Fatal(err)
}
lastCount += 1
expectedPayout1 += DefaultPaymentThreshold + 1
} else {
if err := p1Peer.Send(context.Background(), &testMsgBigPrice{}); err != nil {
t.Fatal(err)
}
if err := waitForChequeProcessed(t, params.backend, counter, lastCount, ts2.swap.peers[p1], expectedPayout2); err != nil {
t.Fatal(err)
}
lastCount += 1
expectedPayout2 += DefaultPaymentThreshold + 1
}
}

ch1, err := ts2.swap.loadLastReceivedCheque(p1)
if err != nil {
t.Fatal(err)
}
ch2, err := ts1.swap.loadLastReceivedCheque(p2)
if err != nil {
t.Fatal(err)
}

expected := uint64(maxCheques) / 2 * (DefaultPaymentThreshold + 1)
if !ch1.CumulativePayout.Equals(uint256.FromUint64(expected)) {
t.Fatalf("expected cumulative payout to be %d, but is %v", expected, ch1.CumulativePayout)
}
if !ch2.CumulativePayout.Equals(uint256.FromUint64(expected)) {
t.Fatalf("expected cumulative payout to be %d, but is %v", expected, ch2.CumulativePayout)
}

log.Info("Simulation ended")
}

// TestMultiChequeSimulation just launches two nodes, a creditor and a debitor.
// The debitor is the one owing to the creditor, so the debitor is the one sending cheques
// It sends multiple cheques in a sequence to the same node.
Expand All @@ -405,18 +277,18 @@ func TestMultiChequeSimulation(t *testing.T) {
defer sim.Close()

log.Info("Initializing")

msgPrice := (&testMsgSmallPrice{}).Price().Value
// we are going to use the metrics system to sync the test
// we are only going to continue with the next iteration after the message
// has been received on the other side
metricsReg := metrics.AccountingRegistry
// testMsgBigPrice is paid by the sender, so the credit counter will only be
// testMsgSmallPrice is paid by the sender, so the credit counter will only be
// increased when receiving the message, which is what we want for this test
cter := metricsReg.Get("account.msg.credit")
counter := cter.(metrics.Counter)
counter.Clear()
var lastCount int64
expectedPayout := DefaultPaymentThreshold + 1
var expectedPayout uint64

_, err = sim.AddNodesAndConnectFull(nodeCount)
if err != nil {
Expand All @@ -434,7 +306,7 @@ func TestMultiChequeSimulation(t *testing.T) {
var debLen, credLen, debSwapLen, credSwapLen int
timeout := time.After(10 * time.Second)
for {
// let's always be nice and allow a time out to be catched
// let's always be nice and allow a time out to be caught
select {
case <-timeout:
t.Fatal("Timed out waiting for all swap peer connections to be established")
Expand All @@ -458,27 +330,74 @@ func TestMultiChequeSimulation(t *testing.T) {
time.Sleep(5 * time.Millisecond)
}

// we will send just maxCheques number of cheques
maxCheques := 10
paymentThreshold := debitorSvc.swap.params.PaymentThreshold

minCheques := 2
maxCheques := 5

msgsPerCheque := (uint64(paymentThreshold) / msgPrice) + 1 // +1 to round up without casting to float

minMsgs := msgsPerCheque * uint64(minCheques)
maxMsgs := msgsPerCheque * uint64(maxCheques)

msgAmount := rand.Intn(int(maxMsgs-minMsgs)) + int(minMsgs)
holisticode marked this conversation as resolved.
Show resolved Hide resolved
log.Debug("sending %d messages", msgAmount)

// the peer object used for sending
debitorSvc.lock.Lock()
creditorPeer := debitorSvc.peers[creditor]
debitorSvc.lock.Unlock()

// send maxCheques number of cheques
for i := 0; i < maxCheques; i++ {
// use a price which will trigger a cheque each time
if err := creditorPeer.Send(context.Background(), &testMsgBigPrice{}); err != nil {
allMessagesArrived := make(chan struct{})

ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second)
defer cancel()

go func() {
for {
select {
case <-ctx.Done():
return
default:
}
// all messages have been received
if counter.Count() == int64(msgAmount) {
close(allMessagesArrived)
return
}
time.Sleep(10 * time.Millisecond)
}
}()

for i := 0; i < msgAmount; i++ {
debitorBalance, err := debitorSvc.swap.loadBalance(creditor)
if err != nil {
t.Fatal(err)
}
// we need to wait a bit in order to give time for the cheque to be processed
if err := waitForChequeProcessed(t, params.backend, counter, lastCount, debitorSvc.swap.peers[creditor], expectedPayout); err != nil {

if err := creditorPeer.Send(context.Background(), &testMsgSmallPrice{}); err != nil {
holisticode marked this conversation as resolved.
Show resolved Hide resolved
t.Fatal(err)
}
lastCount += 1
expectedPayout += DefaultPaymentThreshold + 1

// check if cheque should have been sent
balanceAfterMessage := debitorBalance - int64(msgPrice)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@mortelli mortelli Jan 22, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if balanceAfterMessage <= -paymentThreshold {
// we need to wait a bit in order to give time for the cheque to be processed
if err := waitForChequeProcessed(t, params.backend, counter, lastCount, debitorSvc.swap.peers[creditor], expectedPayout); err != nil {
t.Fatal(err)
}
expectedPayout += uint64(-balanceAfterMessage)
}

lastCount++
}
// give enough time for all messages to be processed
select {
case <-ctx.Done():
t.Fatal("timed out waiting for all messages to arrive, aborting")
case <-allMessagesArrived:
}
log.Debug("all messages arrived")

// check balances:
b1, err := debitorSvc.swap.loadBalance(creditor)
Expand Down Expand Up @@ -507,9 +426,6 @@ func TestMultiChequeSimulation(t *testing.T) {
t.Fatalf("Expected symmetric cheques payout, but they are not: %v vs %v", cheque1.CumulativePayout, cheque2.CumulativePayout)
}

// check also the actual expected amount
expectedPayout = uint64(maxCheques) * (DefaultPaymentThreshold + 1)

if !cheque2.CumulativePayout.Equals(uint256.FromUint64(expectedPayout)) {
t.Fatalf("Expected %d in cumulative payout, got %v", expectedPayout, cheque1.CumulativePayout)
}
Expand Down Expand Up @@ -722,7 +638,7 @@ func waitForChequeProcessed(t *testing.T, backend *swapTestBackend, counter metr
select {
case <-ctx.Done():
lock.Lock()
errs = append(errs, "Timed out waiting for cheque to have been cached")
errs = append(errs, "Timed out waiting for cheque to be cashed")
lock.Unlock()
wg.Done()
return
Expand Down Expand Up @@ -762,7 +678,7 @@ func waitForChequeProcessed(t *testing.T, backend *swapTestBackend, counter metr
select {
case <-ctx.Done():
lock.Lock()
errs = append(errs, "Timed out waiting for peer to have processed accounted message")
errs = append(errs, "Timed out waiting for peer to process accounted message")
lock.Unlock()
wg.Done()
return
Expand Down
7 changes: 7 additions & 0 deletions swap/swap.go
Original file line number Diff line number Diff line change
Expand Up @@ -514,6 +514,13 @@ func (s *Swap) processAndVerifyCheque(cheque *Cheque, p *Peer) (*uint256.Uint256
return nil, err
}

// calculate tentative new balance after cheque is processed
newBalance := p.getBalance() - int64(cheque.Honey)
// check if this new balance would put creditor into debt
if newBalance < -int64(ChequeDebtTolerance) {
return nil, fmt.Errorf("received cheque would result in balance %d which exceeds tolerance %d and would cause debt", newBalance, ChequeDebtTolerance)
}

if err := p.setLastReceivedCheque(cheque); err != nil {
p.logger.Error("error while saving last received cheque", "err", err.Error())
// TODO: what do we do here? Related issue: https://github.com/ethersphere/swarm/issues/1515
Expand Down
Loading