P2p validate accounting #2051

holisticode · 2019-12-17T21:53:28Z

After the roundtable of today, 17.12.2019, one suggestion as on how to handle accounting was made: that a dry-run is performed of the accounting before actually applying ("writing") the accounting.

This PR is a proposal on how to do this.

Before any message operation (sending, receiving), a Check function checks that the actual accounting operation would not result in any error which should prevent the operation to take place.
Only if the Check returns without an error, the operation is performed and then the accounting is applied (Apply).

A lock is now applied on the protocols Peer during this function group, allowing for some kind of atomicity / transactional-like handling.

This PR OR #2049 should be merged.

janos

This is certainly a better approach then #2049. It should be fine for now, but I think that @pradovic suggested, if I am not mistaken, that two phase commit would be more reliable approach. It would have its complexity, but we should see if it is really needed, or validate&apply is good enough.

p2p/protocols/accounting_simulation_test.go

janos · 2019-12-23T13:55:07Z

p2p/protocols/accounting_simulation_test.go

@@ -199,8 +200,9 @@ func (m *matrix) symmetric() error {
 type balances struct {
 	i int
 	*matrix
-	id2n map[enode.ID]int
-	wg   *sync.WaitGroup
+	id2n    map[enode.ID]int


Maybe a bit more descriptive variable name or comment for id2n.

Unfortunately this whole test has been written by @zelig, so motivations for names are not clear to me.

p2p/protocols/accounting_test.go

janos · 2019-12-23T14:00:52Z

p2p/protocols/protocol.go

 	if p.spec.Hook != nil {
-		err := p.spec.Hook.Receive(p, uint32(len(msgBytes)), val)
+


Just a warning, a potential conflict with #2018, but ti should be easy to resolve, whichever PR gets merged first. @pradovic

No problem, thanks for the heads-up!

Thanks for the heads up

mortelli

my first impression here is that i'd rather we move forward with this proposal (rather than with #2049).

do we need the locks in Add and Check if we are already calling the ones at the Send and handleIncoming level?

p2p/protocols/accounting.go

mortelli · 2019-12-19T15:29:20Z

p2p/protocols/accounting.go

-//   - credits/debits local node using balance interface
-func (ah *Accounting) Receive(peer *Peer, size uint32, msg interface{}) error {
+//   - calculates the cost for the local node sending a msg of size to peer querying the message for its price
+func (ah *Accounting) Validate(peer *Peer, size uint32, msg interface{}, payer Payer) (int64, error) {


conceptually, i don't understand the difference between this function and Check, can you please elaborate on this?

this one isn't mentioned in the PR description but it seems to be new

Well Validate is the interface we need to call it from the p2p/protocols package, and does some common work like evaluating the cost for the local node and get the message price.

Check is then implemented by the swap package, and only really does the actual check to see if the operation would incur in some problems in the swap package.

p2p/protocols/accounting.go

holisticode · 2019-12-23T16:12:52Z

The locks here are on the protocols.Peer instance, the ones in Add and Check are on the Swap instance. Even if they are actually the same peer, they are at different levels in the protocol stack, so they are both justified - although in the current situation, Add can not happen without being inside the lock of the procotols peer - but this could actually change...the Swap instance could be called concurrently from other places in the stack (also consider RPC calls).

While I believe it may be safe to omit the locks in the Swap instance for now, I feel it is safer to keep them for the time being. @janos your opinion?

p2p/protocols/protocol.go

ralph-pichler · 2019-12-23T16:34:53Z

swap/swap.go

+
+	swapPeer.lock.Lock()
+	defer swapPeer.lock.Unlock()
+	// we should probably check here again:


Why? With the peer under lock the balance couldn't have changed since the call to Check, right?

That is correct. However, just the nature of the two being separate functions could lead to the situation that Add be called in a different workflow. For example, I do not recall if all tests now do this Validate-Apply workflow - some tests might be calling directly Add (I would actually assume so).

Even if the probability is very low, it is correct to check again. However, I am fine removing the check if a majority thinks it is superfluous.

Hmm. I don't really like this double-check approach. From just looking at it, it looks as if you are doing something redundantly and it is probably also not water-tight.

A different solution could be that the Check function would leave a trace in Swap that a certain amount has been checked and Add only executes on a validated amount. For example, we could create a mapping where each amount maps to a bool (Checked)), to be set to true during the function Check and toggled again after Add has executed.

Might be too much engineering though. Let me know your thoughts!

Removing the additional check, and adding a comment that it is the responsibility of the calling function to check the balance would be more than enough, I think though!

holisticode · 2019-12-23T18:51:12Z

@janos @pradovic two-face commit is a "transactional" design, which I have been proposing as well. We don't have any transactional pattern in the code - I certainly support that.

However, usually transactional patterns are implemented with the help of some library, often at the database layer. Also, when I have been suggesting transactional design it has been generally met with cautiousness, if not rejection.

This PR is some kind of "poor-man" two-face commit, and certainly not a full-fledged implementation of such. Again, I support 2phase commit as the final solution but suggest that for the time being this might be a good start.

janos

LGTM

Eknir · 2020-01-08T10:40:49Z

swap/swap.go

+// It only checks that performing a given accounting operation would not incur in an error.
+// If it returns no error, this signals to the caller that the operation is safe
+func (s *Swap) Check(amount int64, peer *protocols.Peer) (err error) {
+	swapPeer := s.getPeer(peer.ID())


You do s.getPeer(peer.ID()) here and you also do it in the function checkBalance (swap:319)

That's because getPeer is under a lock.

getBalance is called from different places (also from Add) and thus needs to be called independently

I don't understand then why you would call it at this place at all. Seemingly all you are doing is verifying whether the peer is a swapPeer, but this is also the first thing you are doing in checkBalance.
I think that this function can be improved by not doing a call to getPeer at all, or by modifying checkBalance to only accept a swapPeer and then pass the swapPeer to checkBalance.
Let me know if I see it correctly!

I tried to explain before but I'll try again, a bit more explicit.

There are two things here:

Check is called from the p2p/protocols package, which does not know the notion of a Swap peer, thus, Check needs to have a protocols.Peer in its signature - hence, we need a conversion

checkBalance is also called from Add, which also for the same reason has a protocols.Peer in its signature, thus also needs a conversion

the swapPeer is also used elsewhere in the Add function

checkBalance is called independently, from two places.

The other thing is that we are locking the swapPeer during these operations. The lock is on the swapPeer, that is why we need it,

Your optimization suggestions would be more stringent if we can remove the lock from swapPeer in this case as we are already locking the protocols.Peer in the protocols package, as @mortelli also noted. However, I am not 100% sure we can omit that lock, it is the same network remote peer but two different objects (protocols vs swap). I don't feel safe doing this at the moment, which is why I suggest keeping it as-is, but it is a valid suggestion to revisit this part and analyze if the lock can be omitted, in which case we could probably optimize those methods.

However your second suggestion is good, I changed checkBalance to only accept a swap Peer

Eknir · 2020-01-08T10:54:55Z

swap/swap.go

-// Swap implements the protocols.Balance interface
-func (s *Swap) Add(amount int64, peer *protocols.Peer) (err error) {
+// checkBalance checks that the amount would not result in crossing the disconnection threshold
+func (s *Swap) checkBalance(amount int64, peer *protocols.Peer) (err error) {


Nitpicking: you are not just checking the balance here, but checking whether you are allowed to add to the balance. I suggest a name that reflects this such as: checkBalanceAddition

The purpose of calling it like this is that the focus is on checking that the balance is ok; it is irrelevant what and how the balance is checked (in fact I never liked the Add name, which I did not choose).

If something, I would rename it to checkBalanceOk or isBalanceChangeOk or isBalanceChangeAllowed or something like that, but not adding implementation details to the name.

checkBalanceOk sounds like an improvement to me!

Eknir

Good work! I added some comments to clarify things and suggest minor improvements. The test checkAccountingTestCases seems to be wrongly implemented.

Eknir · 2020-01-08T11:13:21Z

swap/swap.go

 	swapPeer := s.getPeer(peer.ID())
 	if swapPeer == nil {
 		return fmt.Errorf("peer %s not a swap enabled peer", peer.ID().String())
 	}
-	swapPeer.lock.Lock()
-	defer swapPeer.lock.Unlock()

 	// check if balance with peer is over the disconnect threshold and if the message would increase the existing debt
 	balance := swapPeer.getBalance()
 	if balance >= s.params.DisconnectThreshold && amount > 0 {


I know you did not introduce this line, but it seems to me that here, we would need to test whether balance + amount < s.params.DisconnectTreshold. If not, the following situation would be allowed:
balance = 499, disconnectThreshold = 500, amount = 1000

But I think we should, in this case, we should disconnect whenever we receive a single message with a positive amount.

To be sure, it would be great to see where and by whom this line was introduced.

This change was introduced by @mortelli with the idea to always allow debt-reducing messages. Let him comment on this; in any case, if this would have to change, it is an important change and then it should be done in a separate PR

I vaguely remember this... This comment is not a blocker for me, but would like to get reassurance from @mortelli :)

indeed, i introduced this change through PR #1922.

the only difference in checking amount instead of balance + amount is one message.

in the first case, one message will go over the threshold and then stop the flow; in the second, the threshold will never be reached or surpassed, but there might be a bigger difference between the threshold and the final balance before subsequent messages are stopped.

while correct, the situation you mentioned is unlikely to come up: if a message is priced at 1000, we should never have set a threshold at 500.

assuming then, that amount is comparatively small in regards to the threshold, the impact of having this little bit of extra slack should be minimal.

p2p/protocols/accounting_test.go

p2p/protocols/protocol.go

Eknir

Some last unresolved comments, but none are a blocker to me.

Unresolved comments:

(redundantly?) doing a call to get a swapPeer
(redundantly?) calling checkBalance

holisticode added the incentives label Dec 17, 2019

holisticode requested review from janos, zelig, ralph-pichler and mortelli December 17, 2019 21:53

holisticode self-assigned this Dec 17, 2019

janos reviewed Dec 23, 2019

View reviewed changes

mortelli reviewed Dec 23, 2019

View reviewed changes

ralph-pichler reviewed Dec 23, 2019

View reviewed changes

janos approved these changes Dec 27, 2019

View reviewed changes

Eknir reviewed Jan 8, 2020

View reviewed changes

Eknir suggested changes Jan 8, 2020

View reviewed changes

Eknir reviewed Jan 8, 2020

View reviewed changes

p2p/protocols/protocol.go Show resolved Hide resolved

Eknir approved these changes Jan 8, 2020

View reviewed changes

holisticode added 7 commits January 8, 2020 11:07

p2p/protocols: change sequence of message accounting

4e226df

p2p/accounting: fix err bug

143a586

p2p/protocols: validate before applying accounting

22b4f59

p2p/protocols: short Sleep to allow accounting to finish

90ebd7e

p2p/protocols: better sync mechanism

3a62941

p2p/protocols: address PR comments

b4f7bd0

swap: addressed PR comments

369cd81

holisticode force-pushed the p2p-validate-accounting branch from 5dfc840 to 369cd81 Compare January 8, 2020 16:08

holisticode merged commit b3d0708 into master Jan 8, 2020

This was referenced Jan 8, 2020

p2p/protocols: change sequence of message accounting #2049

Closed

swap: prevent debt cheques #1983

Merged

acud added this to the 0.5.5 milestone Jan 21, 2020

ralph-pichler mentioned this pull request Feb 19, 2020

accounting lock leads to deadlock with retrieval #2110

Closed

ralph-pichler mentioned this pull request Feb 19, 2020

p2p/protocols: remove peer accounting lock #2112

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

P2p validate accounting #2051

P2p validate accounting #2051

holisticode commented Dec 17, 2019

janos left a comment

janos Dec 23, 2019

holisticode Dec 23, 2019

janos Dec 23, 2019

pradovic Dec 23, 2019

holisticode Dec 23, 2019

mortelli left a comment

mortelli Dec 19, 2019

holisticode Dec 23, 2019

holisticode commented Dec 23, 2019

ralph-pichler Dec 23, 2019

holisticode Dec 23, 2019

Eknir Jan 8, 2020

Eknir Jan 8, 2020

holisticode commented Dec 23, 2019

janos left a comment

Eknir Jan 8, 2020

holisticode Jan 8, 2020

Eknir Jan 8, 2020 •

edited

Loading

holisticode Jan 8, 2020 •

edited

Loading

holisticode Jan 8, 2020

Eknir Jan 8, 2020

holisticode Jan 8, 2020

Eknir Jan 8, 2020

Eknir left a comment

Eknir Jan 8, 2020

Eknir Jan 8, 2020

holisticode Jan 8, 2020

Eknir Jan 8, 2020

mortelli Jan 8, 2020 •

edited

Loading

Eknir left a comment

		if p.spec.Hook != nil {
		err := p.spec.Hook.Receive(p, uint32(len(msgBytes)), val)

P2p validate accounting #2051

P2p validate accounting #2051

Conversation

holisticode commented Dec 17, 2019

janos 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

mortelli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

holisticode commented Dec 23, 2019

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

holisticode commented Dec 23, 2019

janos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eknir Jan 8, 2020 • edited Loading

Choose a reason for hiding this comment

holisticode Jan 8, 2020 • 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

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 Jan 8, 2020 • edited Loading

Choose a reason for hiding this comment

Eknir left a comment

Choose a reason for hiding this comment

Eknir Jan 8, 2020 •

edited

Loading

holisticode Jan 8, 2020 •

edited

Loading

mortelli Jan 8, 2020 •

edited

Loading