swap: load and save received cheques, verify amount and serial are higher #1590

ralph-pichler · 2019-07-22T16:20:21Z

This PR implements the saving, loading and verification of received cheques.

For every cheque received we verify that amount and serial are higher than in the last received cheque and that the amount is compatible with the honey amount specified
Previous checks were refactored into separate methods for easier testing
Different from sent cheques, the last received cheque is stored on Peer not Swap
a signContentWithKey function was added to Swap to also be able to sign cheques with other private keys needed during testing
the default cheque in the tests was replaced with a cheque with timeout 0 to be compatible with Peer
chequeSig was renamed to testChequeSig to make it clearer that it is the sig for newTestCheque

holisticode · 2019-07-24T20:13:54Z

swap/peer.go

-	}
-
-	// TODO: check serial and balance are higher
+	sp.processAndVerifyCheque(cheque)


processAndVerifyCheque returns an error but this is not checked here

You're right, must have forgot to put the check when I extracted the function

holisticode · 2019-07-24T20:15:57Z

swap/peer.go

+		}
+
+		actualAmount -= lastCheque.Amount
+	}


should there be a check that if the lastCheque is nil that then the Serial should be 0 resp. 1 if we start from 1?

We can, but it isn't strictly necessary from the contract side. Any serial != 0 is a valid starting number. But it might be useful to enforce that on the go side. Maybe we should also enforce serials always increasing by 1 then. That way a peer could recognise if it disagrees with the number of cheques sent by the counterparty.

I think I would like that, it makes sense to me. If you are doubting, you can still check back with research though. It may be even more useful for debugging as for the actual cheque handling.

Can you take care of this, to decide if we need this and if yes, to do the needed changes? We can leave it as is for this PR and we can then have a simpler PR.

https://swarmresear.ch/t/swap-and-the-need-for-waivers/40 sounds like we'll get rid of waivers and therefore also serials. So this point will become moot when we update the contract.

holisticode · 2019-07-24T20:22:43Z

swap/peer.go

+
+	// the beneficiary is the owner of the counterparty swap contract
+	if err := sp.swap.verifyChequeSig(cheque, sp.beneficiary); err != nil {
+		log.Error("error invalid cheque", "from", sp.ID().String(), "err", err.Error())


why do we do a log here and in the other cases an fmt.Errorf?

In the other cases we create an error to return. Here we already get an error from verifyChequeSig which we return unmodified (hence no fmt.Errorf). The log was just here for logging purposes, I moved it to processAndVerifyCheque now so that there is one single log call for all kinds of errors.

holisticode · 2019-07-24T20:27:37Z

swap/peer.go

+
+	if err := sp.saveLastReceivedCheque(cheque); err != nil {
+		log.Error("error while saving last received cheque", "peer", sp.ID().String(), "err", err.Error())
+		// TODO: what do we do here?


One option would be to retry several times. I could think of a queue which tries to save it, while at least save it in memory (here the map actually becomes partially useful...)

What do we do with the cached value in the meantime? Right now even if the saving fails the cached value is still updated.

If we retry several times what should happen if we get a new cheque but the old one hasn't even been saved yet?

Let's not discuss this here. This issue has a lot of ramifications and possible solutions, there is a general issue for handling error situations: #1601

We can handle this there, or, if you prefer a dedicated issue just for this case so we don't forget, feel free to create a new issue.

So for this PR should we ignore the error like now (and only update memory) or return the error without updating balances?

Actually this is related to this issue: #1515

If we can't save the cheque, then there is a serious problem with the file system or something, and we should shut down the node. In that case the update to memory would be lost anyway too. So for this PR we can ignore the error, but we need to address this when we do #1515

What about updating memory only if the Put method is successful?

It's a possibility but then the next received cheque (and any that follows) will be rejected as the increase won't match the honey amount.

holisticode · 2019-07-24T20:43:33Z

swap/peer.go

+		return fmt.Errorf("wrong cheque parameters: expected beneficiary: %x, was: %x", sp.swap.owner.address, cheque.Beneficiary)
+	}
+
+	if cheque.Timeout != 0 {


Why are we actually hardcoding the check to be for 0? What's the point of having this field in the cheque if it has to be zero?

Because in theory cheques are allowed (and expected in case waivers are allowed) to have a non-zero timeout. However since we don't use waivers (at least in MVP) and expect increasing amounts on the go side we can do instant cashing which requires a zero timeout.

I would suggest to keep it that way for now until we decide if we get rid of waivers on the contract side. Then we could get rid of timeout, (probably) serial and the 2-step cashing process entirely.

holisticode · 2019-07-24T20:45:53Z

swap/peer.go

+	backend            cswap.Backend
+	beneficiary        common.Address
+	contractAddress    common.Address
+	lastReceivedCheque *Cheque


This is the nicer approach but I wonder, shouldn't we have a consistent handling if you introduce this? Shouldn't we also have lastSentCheque then and have their correspondent methods too?

Yes, that's the idea (move everything that only relates to one peer from swap to peer). But I think that refactor should be done in a separate PR.

I am not sure I agree, because this PR is introducing a change which makes a situation inconsistent which was, even if maybe a bit ugly, consistent before. So I would have nothing against refactoring it inside this same PR.

Nevertheless to speed up things let's move on, I created an issue for this: #1604, but please let's take that issue soon.

I can start working on it as soon as this PR is merged but it's not gonna be ready by the time we want to merge to master.

yes agreed. It will be in after the merge to master - unless reviewers of the master PR object for some reason

holisticode · 2019-07-24T20:54:42Z

swap/swap_test.go

+}
+
+// TestPeerSaveAndLoadLastReceivedCheque tests if a saved last received cheque can be loaded again later using the peer functions
+func TestPeerSaveAndLoadLastReceivedCheque(t *testing.T) {


Maybe this is redundant, why is it needed to also test via peer functions?

I would keep both because the peer test wouldn't catch if the saving was implemented incorrectly (as it would still retrieve the correct cached value). The swap test on the other hand wouldn't catch an error in the caching.

Sorry, accidentally hit the resolve button

holisticode · 2019-07-24T20:58:17Z

swap/swap_test.go

+
+	if err := peer.verifyChequeAgainstLast(newCheque, oldCheque, increase); err == nil {
+		t.Fatal("accepted a cheque with unexpected amount")
+	}


Great tests, do we need to add one if there is actually no existing cheque (first one) or is this covered?

This particular test tests the verifyChequeAgainstLast function which is only ever called if there was a prior cheque.

What is missing is a test for processAndVerifyCheque with an invalid cheque if it is the first cheque. I will add one. (has been added to the TestPeerProcessAndVerifyChequeInvalid test)

holisticode

This is a great PR! Adds much needed robustness to the cheque handling.

Please address the remaining comments and remove the DRAFT status.
It is probably also best to create it onto incentives rather than incentives-prices

holisticode

LGTM now! Thank you.

I would suggest you invite one more person more from our team maybe?

holisticode · 2019-07-25T12:35:11Z

swap/peer.go

+	backend            cswap.Backend
+	beneficiary        common.Address
+	contractAddress    common.Address
+	lastReceivedCheque *Cheque


I am not sure I agree, because this PR is introducing a change which makes a situation inconsistent which was, even if maybe a bit ugly, consistent before. So I would have nothing against refactoring it inside this same PR.

Nevertheless to speed up things let's move on, I created an issue for this: #1604, but please let's take that issue soon.

holisticode · 2019-07-25T12:42:27Z

swap/peer.go

+
+	if err := sp.saveLastReceivedCheque(cheque); err != nil {
+		log.Error("error while saving last received cheque", "peer", sp.ID().String(), "err", err.Error())
+		// TODO: what do we do here?


Let's not discuss this here. This issue has a lot of ramifications and possible solutions, there is a general issue for handling error situations: #1601

We can handle this there, or, if you prefer a dedicated issue just for this case so we don't forget, feel free to create a new issue.

holisticode · 2019-07-25T12:51:04Z

swap/peer.go

+		}
+
+		actualAmount -= lastCheque.Amount
+	}


I think I would like that, it makes sense to me. If you are doubting, you can still check back with research though. It may be even more useful for debugging as for the actual cheque handling.

Can you take care of this, to decide if we need this and if yes, to do the needed changes? We can leave it as is for this PR and we can then have a simpler PR.

mortelli · 2019-07-25T14:22:58Z

swap/peer.go

+
+	// TODO: maybe allow some range around expectedAmount?
+	if expectedAmount != actualAmount {
+		return fmt.Errorf("unexpected exchange rate used for honey, expected %d was %d", expectedAmount, actualAmount)


I'm not sure I understand this error message. The exchange rate could have been different since the expected amount differs from the actual amount, right?

But then you are showing the amounts in the error message, not the exchange rates, no?

True, how about fmt.Errorf("unexpected amount for honey, expected %d was %d", expectedAmount, actualAmount)?

Sounds good to me!

swap/peer.go

mortelli · 2019-07-25T14:27:58Z

swap/swap.go

+func (s *Swap) loadLastReceivedCheque(peer enode.ID) *Cheque {
+	s.lock.Lock()
+	defer s.lock.Unlock()
+	var cheque *Cheque


Minor: I think you can save 1 line here by using a named return parameter.

mortelli · 2019-07-25T14:33:43Z

swap/swap_test.go

+		t.Fatalf("Could not find saved cheque")
+	}
+
+	if returnedCheque.Amount != testCheque.Amount || returnedCheque.Beneficiary != testCheque.Beneficiary {


Is there any reason why you are checking only these 2 fields, rather than all of them? (I was thinking of something like an equals method)

Mainly because there is no equals function yet. The idea behind these 2 fields was that if those 2 were set correctly the rest most likely was as well.

I'm actually working on a separate PR where I extract the cheque related functions from swap into their own package. There I will add an equal and then update the tests.

mortelli · 2019-07-25T14:36:06Z

swap/swap_test.go

+		t.Fatal("Could not find saved cheque")
+	}
+
+	if returnedCheque.Amount != testCheque.Amount || returnedCheque.Beneficiary != testCheque.Beneficiary {


Is there any reason why you are checking only these 2 fields, rather than all of them? (I was thinking of something like an equals method)

(same here)

mortelli

Found no major issues. Other than the comments I've left, LGTM 👍

…gher (#1590) * swap: compute actual payment amount and verify honey amount and serial * swap: add tests for cheque verification * address pr comments * address PR comments

ralph-pichler force-pushed the incentives_verify-amount-serial branch from d3a7978 to 31adc91 Compare July 22, 2019 16:21

ralph-pichler changed the title ~~load and save received cheques, verify amount and serial are higher~~ swap: load and save received cheques, verify amount and serial are higher Jul 22, 2019

ralph-pichler force-pushed the incentives_verify-amount-serial branch 9 times, most recently from 6939b76 to 86a2c27 Compare July 24, 2019 18:34

holisticode added the incentives label Jul 24, 2019

holisticode reviewed Jul 24, 2019

View reviewed changes

ralph-pichler added 3 commits July 25, 2019 12:50

swap: compute actual payment amount and verify honey amount and serial

8f5ecc7

swap: add tests for cheque verification

16b0660

address pr comments

a6053c6

ralph-pichler force-pushed the incentives_verify-amount-serial branch from 87db581 to a6053c6 Compare July 25, 2019 10:51

ralph-pichler changed the base branch from incentives-prices to incentives July 25, 2019 10:51

ralph-pichler marked this pull request as ready for review July 25, 2019 10:53

holisticode mentioned this pull request Jul 25, 2019

Refactor lastSentCheque according to lastReceivedCheque #1604

Closed

holisticode approved these changes Jul 25, 2019

View reviewed changes

holisticode assigned ralph-pichler Jul 25, 2019

ralph-pichler requested a review from mortelli July 25, 2019 13:37

mortelli reviewed Jul 25, 2019

View reviewed changes

swap/peer.go Show resolved Hide resolved

mortelli reviewed Jul 25, 2019

View reviewed changes

swap/peer.go Show resolved Hide resolved

mortelli reviewed Jul 25, 2019

View reviewed changes

address PR comments

998d8ac

mortelli approved these changes Jul 25, 2019

View reviewed changes

holisticode merged commit b894062 into incentives Jul 25, 2019

ralph-pichler deleted the incentives_verify-amount-serial branch July 25, 2019 17:33

swap: load and save received cheques, verify amount and serial are higher #1590

swap: load and save received cheques, verify amount and serial are higher #1590

Conversation

ralph-pichler commented Jul 22, 2019 • edited 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

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

ralph-pichler Jul 25, 2019 • edited Loading

Choose a reason for hiding this comment

holisticode left a comment • edited Loading

Choose a reason for hiding this comment

holisticode 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

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

mortelli left a comment

Choose a reason for hiding this comment

ralph-pichler commented Jul 22, 2019 •

edited

Loading

ralph-pichler Jul 25, 2019 •

edited

Loading

holisticode left a comment •

edited

Loading