swap: introduce pending cheques, drop peers on io errors #1887

ralph-pichler · 2019-10-18T02:41:34Z

This PR implements the pending cheque mechanism as described here (#1601 (comment)).
There are two differences:

Once a cheque is confirmed the node will immediately recheck wether to send another one.
The balance on the sender side is adjusted before sending the cheque.

Eknir · 2019-10-18T07:51:58Z

swap/peer.go

@@ -40,6 +40,7 @@ type Peer struct {
 	contractAddress    common.Address
 	lastReceivedCheque *Cheque
 	lastSentCheque     *Cheque
+	pendingCheque      *Cheque


A small comment to each field in the peer struct would be nice.

Eknir · 2019-10-18T08:26:00Z

swap/swap.go

+	}
+
+	if err := p.setLastSentCheque(cheque); err != nil {
+		p.Drop("persistence error")


Why would we drop the peer if our node cannot save the cheque? I would propose to shut down Swarm with this i/o error.

It's not entirely clear to me how to shut down swarm and I didn't want to ignore the error entirely, so I used drop because that's what the other protocols do on io error as well.

Requesting @janos to give his opinion here.
After an IO error, is it sufficient to just drop the peer (as done by @ralph-pichler here) or should Swarm be stopped entirely? If we need to stop Swarm, how to do this safely?

we don't support a shutdown invoked by some high level components. you could panic and risk data corruption with the databases since a shutdown is not invoked when panicking. dropping the peer in this case is the safer option because that way, if you're getting local IO errors and you start dropping peers - you isolate the problem to your local node and technically you do not participate in the network anymore (ideally). I'd stick with dropping peers at the moment since node behavior with IO errors is something we have not tested yet and it is quite an involved task in its own

A very interesting question. I agree with @acud, as we do not have a way to shut down the node, except with panic. If this error happens, we will drop the peer, but it is very likely that the same I/O error will happen in all next calls, so we will drop all peers, practically removing our node from the network. And maybe recovering if the I/O problem gets resolved. This could happen if the disk is full, for example. Maybe we could drop all peers if this happens which would prevent other peers to step into the same problem. I am not sure how easy is to do so currently. If it requires non trivial changes, this drop is ok for now.

swap/swap.go

Eknir · 2019-10-18T08:51:33Z

swap/peer.go

@@ -154,16 +169,18 @@ func (p *Peer) createCheque() (*Cheque, error) {
 // To be called with mutex already held


Could you update the comment, mentioning the added functionality?

Eknir · 2019-10-18T08:58:07Z

swap/swap.go

+	}
+
+	// since more swap traffic might have occurred since this cheque was already sent, we redo the payment threshold check
+	if err := s.checkPaymentThreshold(p); err != nil {


If the disconnect threshold is set at less than 200% of the payment threshold, this function will never result in sending another cheque.

Proposing to update the comment:
since more swap traffic might have occurred (when disconnect threshold >= 2 * paymentThreshold)...

is it possible that more traffic has occurred when this function is under lock?

Not during handleConfirmChequeMsg. But traffic might have occurred since the call to checkPaymentThreshold in Add where this cheque was initially created.

Eknir · 2019-10-18T09:08:26Z

swap/swap.go

+	}
+
+	// since more swap traffic might have occurred since this cheque was already sent, we redo the payment threshold check
+	if err := s.checkPaymentThreshold(p); err != nil {


If sendCheque returns an error here, it is handled differently than when sendCheque returns an error while being called from the Add function. I don't think this is desirable.

I agree. I intentionally didn't change the error handling in Add in this PR to keep it simple. Add shouldn't return an error either if cheque sending fails but I would leave that (and the rest of the errors issues in Add) to a separate PR and only deal with pending cheques in this one.

Eknir · 2019-10-18T09:09:48Z

swap/types.go

@@ -43,3 +43,7 @@ type HandshakeMsg struct {
 type EmitChequeMsg struct {
 	Cheque *Cheque
 }
+


Please comment

Eknir · 2019-10-18T09:17:30Z

swap/protocol_test.go

@@ -251,6 +251,10 @@ func TestTriggerPaymentThreshold(t *testing.T) {
 		t.Fatal(err)
 	}

+	debitorSwap.handleConfirmChequeMsg(ctx, creditor, &ConfirmChequeMsg{


I would place these additional lines line just above the lines containing:

if creditor.getBalance() != 0 { t.Fatalf("Expected debitorSwap balance to be 0, but is %d", creditor.getBalance()) }

Not sure what you mean here. Move which lines where exactly?

Eknir · 2019-10-18T09:25:13Z

swap/swap.go

@@ -198,6 +198,7 @@ const (
 	balancePrefix          = "balance_"
 	sentChequePrefix       = "sent_cheque_"
 	receivedChequePrefix   = "received_cheque_"
+	pendingChequePrefix    = "pending_cheque_"


Can you add this new key to the test TestStoreKeys in swap_test.go?

Eknir

Good work. This makes SWAP much more robust.

I left a few comments.

Apart from those, I think it would be good to have test scenario's covering (make new tests or add to existing tests):
handleEmit:

After receiving a new cheque, a confirm message is sent, containing the new cheque
After receiving an already sent cheque, a confirm message is sent, containing the old cheque
handleConfirm
Balance is updated
sentCheque
balance is not updated before getting the confirm message

swap/peer.go

swap/swap.go

acud · 2019-10-21T09:00:07Z

swap/swap.go

+	}
+
+	if err := p.setLastSentCheque(cheque); err != nil {
+		p.Drop("persistence error")


we don't support a shutdown invoked by some high level components. you could panic and risk data corruption with the databases since a shutdown is not invoked when panicking. dropping the peer in this case is the safer option because that way, if you're getting local IO errors and you start dropping peers - you isolate the problem to your local node and technically you do not participate in the network anymore (ideally). I'd stick with dropping peers at the moment since node behavior with IO errors is something we have not tested yet and it is quite an involved task in its own

acud · 2019-10-21T09:02:16Z

swap/swap.go

@@ -318,6 +336,12 @@ func (s *Swap) handleEmitChequeMsg(ctx context.Context, p *Peer, msg *EmitCheque
 		return err
 	}

+	if err := p.Send(ctx, &ConfirmChequeMsg{


@ralph-pichler the convention to use is:

err := p.Send(...) if err != nil { return err }

for all error checks in the same block, this way you prevent allocating a new variable for every function call that checks for an error. please fix it in the calls below too

I changed it for handleConfirmChequeMsg and handleEmitChequeMsg. Hope that's ok now, I remember the opposite recommendation from other PRs.

acud · 2019-10-21T09:04:50Z

swap/swap.go

+	}
+
+	// since more swap traffic might have occurred since this cheque was already sent, we redo the payment threshold check
+	if err := s.checkPaymentThreshold(p); err != nil {


is it possible that more traffic has occurred when this function is under lock?

mortelli

no major gripes, looks good overall.

i'll take another look after having the discussions addressed.

i recommend rebasing/merging with master often to avoid falling behind too much and having to deal with complicated conflicts

swap/peer.go

swap/swap.go

mortelli

even though not all my comments have been addressed, i am going to go ahead and approve this PR, as i don't perceive any of them as blockers 👍

mortelli

i am OK with the change to the moment in which the paying node adjusts its balance. it doesn't directly solve the problem (i.e. prevent it completely) but it makes it harder to occur.

aside from that, please address the open threads from my previous reviews so they can be resolved

swap/swap.go

ralph-pichler added incentives builds on open PR Builds on a PR that is not yet merged. It is blocked until then and the diff won't make sense yet. labels Oct 18, 2019

ralph-pichler force-pushed the swap-pending-cheques branch from 196f0b3 to 96f540a Compare October 18, 2019 02:47

ralph-pichler changed the base branch from swap-factory to master October 18, 2019 02:47

ralph-pichler added ready for review and removed builds on open PR Builds on a PR that is not yet merged. It is blocked until then and the diff won't make sense yet. labels Oct 18, 2019

ralph-pichler force-pushed the swap-pending-cheques branch 2 times, most recently from 1e4be68 to 75bf887 Compare October 18, 2019 03:01

ralph-pichler requested review from Eknir and mortelli October 18, 2019 03:09

Eknir reviewed Oct 18, 2019

View reviewed changes

swap/swap.go Outdated Show resolved Hide resolved

Eknir reviewed Oct 18, 2019

View reviewed changes

swap/swap.go Outdated Show resolved Hide resolved

Eknir reviewed Oct 18, 2019

View reviewed changes

acud suggested changes Oct 21, 2019

View reviewed changes

ralph-pichler removed the ready for review label Oct 22, 2019

ralph-pichler force-pushed the swap-pending-cheques branch from e96d98f to 8fcc32c Compare October 23, 2019 15:12

ralph-pichler added 5 commits October 24, 2019 16:57

swap: introduce pending cheques, drop peers on io errors

c872714

swap: return if confirm while no pending, add comments to peer getters

c8aebdd

swap: add comments for pending cheques

ff1bbfc

swap: add pending cheque key to TestStoreKeys

ea75197

swap: use same err in same block

b80884a

ralph-pichler force-pushed the swap-pending-cheques branch from 8fcc32c to b80884a Compare October 24, 2019 14:58

mortelli reviewed Oct 28, 2019

View reviewed changes

ralph-pichler added 3 commits October 28, 2019 16:32

swap: adjust comments and error message

af27bb8

swap: dont crash if there is no last received cheque

afda8db

swap: improve logs and error messages

0d20163

mortelli approved these changes Oct 30, 2019

View reviewed changes

This was referenced Oct 31, 2019

Exchange lastCheque #1640

Closed

always allow debt-reducing messages #1921

Closed

swap: always allow debt-reducing messages #1922

Merged

threshold handling during message processing? #1506

Open

swap: prevent bad cheques from being sent #1925

Draft

Merge remote-tracking branch 'origin/master' into swap-pending-cheques

927a239

mortelli assigned mortelli and ralph-pichler and unassigned mortelli Nov 4, 2019

swap: add pending cheques to sentcheques and availablebalance

e22d28e

ralph-pichler force-pushed the swap-pending-cheques branch from 0c67a7c to e22d28e Compare November 4, 2019 13:02

ralph-pichler added 2 commits November 4, 2019 15:29

swap: include pending cheques in tests

355ba72

swap: adjust balance before sending the cheque

a8b3cd1

ralph-pichler requested a review from mortelli November 4, 2019 14:46

ralph-pichler added the ready for another review label Nov 4, 2019

mortelli reviewed Nov 4, 2019

View reviewed changes

swap/swap.go Outdated Show resolved Hide resolved

ralph-pichler added 5 commits November 4, 2019 19:48

swap: return nil in case of error in load functions

c9cef00

swap: implement String on cheque

fa753a6

swap: test more cheque fields in TestTriggerPaymentThreshold

5d7bfd8

swap: remove checkPaymentThresholdAndSendCheque on confirm message

e8b7c94

Merge remote-tracking branch 'origin/master' into swap-pending-cheques

021fc2b

ralph-pichler merged commit 59b2025 into master Nov 4, 2019

mortelli deleted the swap-pending-cheques branch November 5, 2019 12:24

acud added this to the 0.5.3 milestone Nov 25, 2019

mortelli mentioned this pull request Jan 7, 2020

shut down node when I/O error occurs during accounting #1515

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

swap: introduce pending cheques, drop peers on io errors #1887

swap: introduce pending cheques, drop peers on io errors #1887

ralph-pichler commented Oct 18, 2019 •

edited

Loading

Eknir Oct 18, 2019

ralph-pichler Oct 22, 2019

Eknir Oct 18, 2019 •

edited by acud

Loading

ralph-pichler Oct 18, 2019

Eknir Oct 21, 2019

acud Oct 21, 2019

janos Oct 21, 2019

mortelli Oct 25, 2019

Eknir Oct 18, 2019

ralph-pichler Oct 22, 2019

Eknir Oct 18, 2019

acud Oct 21, 2019

ralph-pichler Oct 22, 2019

Eknir Oct 18, 2019

ralph-pichler Oct 22, 2019

Eknir Oct 18, 2019

ralph-pichler Oct 22, 2019

Eknir Oct 18, 2019

ralph-pichler Oct 22, 2019

Eknir Oct 18, 2019 •

edited

Loading

ralph-pichler Oct 22, 2019

Eknir left a comment

acud Oct 21, 2019

acud Oct 21, 2019

ralph-pichler Oct 22, 2019

acud Oct 21, 2019

mortelli left a comment

mortelli left a comment

mortelli left a comment •

edited

Loading

		@@ -154,16 +169,18 @@ func (p Peer) createCheque() (Cheque, error) {
		// To be called with mutex already held

swap: introduce pending cheques, drop peers on io errors #1887

swap: introduce pending cheques, drop peers on io errors #1887

Conversation

ralph-pichler commented Oct 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eknir Oct 18, 2019 • edited by acud Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eknir Oct 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eknir left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mortelli left a comment

Choose a reason for hiding this comment

mortelli left a comment

Choose a reason for hiding this comment

mortelli left a comment • edited Loading

Choose a reason for hiding this comment

ralph-pichler commented Oct 18, 2019 •

edited

Loading

Eknir Oct 18, 2019 •

edited by acud

Loading

Eknir Oct 18, 2019 •

edited

Loading

mortelli left a comment •

edited

Loading