swap: always allow debt-reducing messages #1922
Conversation
mortelli
added
in progress
incentives
builds on open PR
mortelli
changed the title
…allow-debt-reduction
…allow-debt-reduction
mortelli
removed
the
builds on open PR
…uction # Conflicts: # swap/protocol_test.go # swap/swap.go # swap/swap_test.go
| @@ -302,8 +302,8 @@ func (s *Swap) Add(amount int64, peer *protocols.Peer) (err error) { | |||
|
|
|||
| // Check if balance with peer is over the disconnect threshold | |||
| balance := swapPeer.getBalance() | |||
| if balance >= s.params.DisconnectThreshold { | |||
| return fmt.Errorf("balance for peer %s is over the disconnect threshold %d, disconnecting", peer.ID().String(), s.params.DisconnectThreshold) | |||
There was a problem hiding this comment.
I wonder how useful this is without also preventing the sending of debt-incurring messages on the other node. If only a single debt incurring message arrives we immediately disconnect (and the other node may not even be aware of the fact that it is now above the threshold if the balances are not in perfect sync).
There was a problem hiding this comment.
i guess we could prevent sending debt-increasing messages when near the disconnect threshold.
that would have the consequence of never dropping peers (if nodes do not behave maliciously)... not sure if we want this or not.
also: what we do when returning an error here is probably best to keep in mind when we deal with the changes to the retrieve protocol, error handling, the blacklist and such (meaning: if we really drop a peer after returning this error or not)
There was a problem hiding this comment.
This is definitely not useful if there would actually be a disconnect at the disconnect threshold and if that disconnect threshold would be triggered more than once.
We need to implement on the requesting node that it won't send any debt-incurring messages after the payment threshold (note: not the disconnect threshold, we need this buffer) has been reached and there are no funds to pay.
There was a problem hiding this comment.
For now, I would say it is useful and we should perhaps leave a comment in the code to address this issue when at a later point the error indeed causes a disconnect
swap/swap.go
Outdated
| @@ -302,8 +302,8 @@ func (s *Swap) Add(amount int64, peer *protocols.Peer) (err error) { | |||
|
|
|||
| // Check if balance with peer is over the disconnect threshold | |||
There was a problem hiding this comment.
Can you update this comment?
i wouldn't rush with this one, although maybe it's good to have an issue created to remember this discussion. but i am thinking that maybe this would affect forwarding, no? |
There was a problem hiding this comment.
Approved.
But indeed we need to create an issue, and although it is not highest priority, it is also not just to "remember this discussion" - it is something we have to do.
The creditor node also needs to keep track of the disconnect threshold and prevent a debit-incurring message in the first place, because at the moment where it locally accounts, but the peer disconnects, we need to consider the balances out-of-sync. While this may be ok short term, it can be irrecoverable if the nodes later resume (for example because the other paid the cheque).
Also to note here is that if the message is Receiver pays, and the peer would cross the disconnect threshold, then this not only means that the peer is disconnected, but also that the original message is not sent (in this case the other peer doesn't even ever account for it) - just informative..
I understand your point, but I am not immediately convinced by this. This also—I think—ties in to what @ralph-pichler commented here. If we prevent the debitor node from sending messages which would put it over the disconnect threshold, wouldn't this prevent peers being dropped/disconnected in most cases? I am not aware of this being necessarily what we want. In fact, didn't we implement the disconnect threshold to drop bad peers in the first place? (leaving aside the technical details of protocols which prevent this from actually happening in some cases). Also, it's still not clear to me how it would affect forwarding.
I think in this case we don't account for it either, but you are right that the peer would be dropped. Besides this, I think @Eknir raised a different issue which can also be discussed:
|
|
@holisticode in any case, I think something similar/equivalent to what you are saying is described in Situation 1 here. Is this correct? If so, it's still an open problem we need to solve. And the suggestion of preventing messages from the debitor which would put it over the disconnect threshold in the creditor would mitigate this. |
|
@mortelli yes, it's described there and the suggestion would address it. That is why I would like to see an issue for this. Again, maybe not highest priority, but certainly important. |
This PR changes the SWAP
Addfunction so that debt-reducing messages are always processed.It also updates the relevant tests.
closes #1921