feat: Always reset balances to zero after disconnection or blocklisting #1983
Conversation
metacertain
force-pushed
the
reset_balances
branch
3 times, most recently
from
1419053
to
ac3d552
Compare
| return a.p2p.Blocklist(peer, time.Duration(disconnectFor)*time.Second) | ||
| } | ||
|
|
||
| func (a *Accounting) Connect(peer swarm.Address) { |
There was a problem hiding this comment.
Maybe we can unit test these functions?
There was a problem hiding this comment.
yes, planning to add a unit test still, thanks!
| } | ||
| } | ||
|
|
||
| func (a *Accounting) Disconnect(peer swarm.Address) { |
There was a problem hiding this comment.
Maybe we can unit test these functions?
metacertain
force-pushed
the
reset_balances
branch
2 times, most recently
from
97df5fe
to
6fa570f
Compare
metacertain
force-pushed
the
reset_balances
branch
from
16c5f31
to
20aa5ae
Compare
| @@ -102,6 +102,7 @@ func (s *Service) init(ctx context.Context, p p2p.Peer) error { | |||
| s.peers[p.Address.String()] = peerData | |||
| } | |||
|
|
|||
| s.accounting.Connect(p.Address) | |||
There was a problem hiding this comment.
Question here is whether the connect reseting balances can race with first push / retrieve requests being reserved / credited / debited, so if we have traffic immediately from the peer, can it change any balances before they are reset creating discrepancies
There was a problem hiding this comment.
Reviewed 2 of 8 files at r1, 1 of 3 files at r2, 1 of 5 files at r3, 5 of 5 files at r4.
Reviewable status: 9 of 10 files reviewed, 6 unresolved discussions (waiting on @anatollupacescu, @esadakar, and @metacertain)
pkg/accounting/accounting.go, line 94 at r4 (raw file):
refreshTimestamp int64 // last time we attempted time-based settlement paymentOngoing bool // indicate if we are currently settling with the peer reconnectAllowTimestamp int64
comments explaining the fields
pkg/settlement/swap/swap_test.go, line 86 at r4 (raw file):
} func (t *testObserver) Connect(peer swarm.Address) {
can we just introduce Connect and Disconnect in a Connecter interface only required where it is used
This PR introduces a failsafe mechanism for unforeseeable problems that can result in accounting discrepancies and enabling nodes to be able to recover a state of symmetric balances afterwards while inhibiting nodes of taking advantage of the failsafe mechanism.
The failsafe mechanism is that both peers start with 0 balances upon a new establishment of the connection in a peer to peer relationship.
The safety of this mechanism is guaranteed by not allowing reconnections by the same peer until the refreshement rate would remove the outstanding debt plus one times paymentthreshold worth of credit space, in case a peer disconnects from us.
This pr also introduces ghost debt, meaning we account for requests that the node attempted to serve as a forwarder - meaning it successfully requested it and got a response for the same request from a third peer - but, for some reason, was unable to write back the result and thereby could not actually debit for these 'ghosted' requests.
Once the cumulative amount of ghost balance reaches the disconnect limit, the peer in ghost debt is blocklisted by the node based on the time the refreshement rate would take to remove the outstanding debt plus one times paymentthreshold worth of credit space.
For all such blocklisting purposes, the outstanding debt is defined as the sum of:
This change is