-
Notifications
You must be signed in to change notification settings - Fork 110
swap: load and save received cheques, verify amount and serial are higher #1590
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -36,10 +36,11 @@ var ErrDontOwe = errors.New("no negative balance") | |
// Peer is a devp2p peer for the Swap protocol | ||
type Peer struct { | ||
*protocols.Peer | ||
swap *Swap | ||
backend cswap.Backend | ||
beneficiary common.Address | ||
contractAddress common.Address | ||
swap *Swap | ||
backend cswap.Backend | ||
beneficiary common.Address | ||
contractAddress common.Address | ||
lastReceivedCheque *Cheque | ||
} | ||
|
||
// NewPeer creates a new swap Peer instance | ||
|
@@ -76,26 +77,11 @@ func (sp *Peer) handleEmitChequeMsg(ctx context.Context, msg *EmitChequeMsg) err | |
log.Info("received emit cheque message") | ||
|
||
cheque := msg.Cheque | ||
if cheque.Contract != sp.contractAddress { | ||
return fmt.Errorf("wrong cheque parameters: expected contract: %s, was: %s", sp.contractAddress, cheque.Contract) | ||
} | ||
|
||
// the beneficiary is the owner of the counterparty swap contract | ||
if err := sp.swap.verifyChequeSig(cheque, sp.beneficiary); err != nil { | ||
if err := sp.processAndVerifyCheque(cheque); err != nil { | ||
log.Error("error invalid cheque", "from", sp.ID().String(), "err", err.Error()) | ||
return err | ||
} | ||
|
||
if cheque.Beneficiary != sp.swap.owner.address { | ||
return fmt.Errorf("wrong cheque parameters: expected beneficiary: %s, was: %s", sp.swap.owner.address, cheque.Beneficiary) | ||
} | ||
|
||
if cheque.Timeout != 0 { | ||
return fmt.Errorf("wrong cheque parameters: expected timeout to be 0, was: %d", cheque.Timeout) | ||
} | ||
|
||
// TODO: check serial and balance are higher | ||
|
||
// reset balance by amount | ||
// as this is done by the creditor, receiving the cheque, the amount should be negative, | ||
// so that updateBalance will calculate balance + amount which result in reducing the peer's balance | ||
|
@@ -137,3 +123,93 @@ func (sp *Peer) handleErrorMsg(ctx context.Context, msg interface{}) error { | |
// maybe balance disagreement | ||
return nil | ||
} | ||
|
||
// processAndVerifyCheque verifies the cheque and compares it with the last received cheque | ||
// if the cheque is valid it will also be saved as the new last cheque | ||
func (sp *Peer) processAndVerifyCheque(cheque *Cheque) error { | ||
if err := sp.verifyChequeProperties(cheque); err != nil { | ||
return err | ||
} | ||
|
||
lastCheque := sp.loadLastReceivedCheque() | ||
|
||
// TODO: there should probably be a lock here? | ||
expectedAmount, err := sp.swap.oracle.GetPrice(cheque.Honey) | ||
if err != nil { | ||
return err | ||
} | ||
|
||
if err := sp.verifyChequeAgainstLast(cheque, lastCheque, expectedAmount); err != nil { | ||
return err | ||
} | ||
|
||
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? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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...) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So for this PR should we ignore the error like now (and only update memory) or return the error without updating balances? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about updating memory only if the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
return nil | ||
} | ||
|
||
// verifyChequeProperties verifies the signautre and if the cheque fields are appropiate for this peer | ||
// it does not verify anything that requires knowing the previous cheque | ||
func (sp *Peer) verifyChequeProperties(cheque *Cheque) error { | ||
if cheque.Contract != sp.contractAddress { | ||
return fmt.Errorf("wrong cheque parameters: expected contract: %x, was: %x", sp.contractAddress, cheque.Contract) | ||
} | ||
|
||
// the beneficiary is the owner of the counterparty swap contract | ||
if err := sp.swap.verifyChequeSig(cheque, sp.beneficiary); err != nil { | ||
return err | ||
} | ||
|
||
if cheque.Beneficiary != sp.swap.owner.address { | ||
return fmt.Errorf("wrong cheque parameters: expected beneficiary: %x, was: %x", sp.swap.owner.address, cheque.Beneficiary) | ||
} | ||
|
||
if cheque.Timeout != 0 { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
return fmt.Errorf("wrong cheque parameters: expected timeout to be 0, was: %d", cheque.Timeout) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// verifyChequeAgainstLast verifies that serial and amount are higher than in the previous cheque | ||
// furthermore it cheques that the increase in amount is as expected | ||
func (sp *Peer) verifyChequeAgainstLast(cheque *Cheque, lastCheque *Cheque, expectedAmount uint64) error { | ||
actualAmount := cheque.Amount | ||
|
||
if lastCheque != nil { | ||
if cheque.Serial <= lastCheque.Serial { | ||
return fmt.Errorf("wrong cheque parameters: expected serial larger than %d, was: %d", lastCheque.Serial, cheque.Serial) | ||
} | ||
|
||
if cheque.Amount <= lastCheque.Amount { | ||
return fmt.Errorf("wrong cheque parameters: expected amount larger than %d, was: %d", lastCheque.Amount, cheque.Amount) | ||
} | ||
|
||
actualAmount -= lastCheque.Amount | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. should there be a check that if the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
||
// TODO: maybe allow some range around expectedAmount? | ||
if expectedAmount != actualAmount { | ||
return fmt.Errorf("unexpected amount for honey, expected %d was %d", expectedAmount, actualAmount) | ||
} | ||
|
||
return nil | ||
} | ||
|
||
// loadLastReceivedCheque gets the last received cheque for this peer | ||
// cheque gets loaded from database if not already in memory | ||
func (sp *Peer) loadLastReceivedCheque() *Cheque { | ||
if sp.lastReceivedCheque == nil { | ||
sp.lastReceivedCheque = sp.swap.loadLastReceivedCheque(sp.ID()) | ||
mortelli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
return sp.lastReceivedCheque | ||
} | ||
|
||
// saveLastReceivedCheque saves cheque as the last received cheque for this peer | ||
func (sp *Peer) saveLastReceivedCheque(cheque *Cheque) error { | ||
sp.lastReceivedCheque = cheque | ||
return sp.swap.saveLastReceivedCheque(sp.ID(), cheque) | ||
mortelli marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes agreed. It will be in after the merge to
master
- unless reviewers of themaster
PR object for some reason