p2p/protocols: change sequence of message accounting #2049
Conversation
There was a problem hiding this comment.
(requesting changes due to a small correction)
this PR surprises me a little bit since it seems like a no-brainer, so i am wondering why we hadn't thought of this up until now 😄 in principle, it seems like a good idea.
my main question here is: why not apply this same change to the handleIncoming function?
i would additionally tag @janos, @zelig, @acud or someone else who could give their opinion regarding a change to the protocols package (i know this would go into another open PR, but it's probably easier for them to take a look in this one).
|
I find it also strange that this problem was not noticed earlier. Thanks @holisticode for a great find. The change is small, but significant and fine, except the return nil, instead the error value. I see no conflicts with any ongoing work in protocols package. |
|
making a note of @ralph-pichler's comment: if this change is applied we won't be able to drop already-sent messages through accounting logic, which is something we could do (and were doing) with the code as it is in with this ordering we can only take steps to prevent further messages from being sent, drop the peer, or maybe some other after-the-fact measure. as @zelig points out (and i agree with him on this) it would be 1 or just a few messages which would "slip by" until the resulting action takes place when the accounting code executes right after a message that shouldn't have been sent according to the hook. i still prefer this alternative to accounting-first-and-then-sending, which also sets up the node for accounting differences. |
There was a problem hiding this comment.
i want to approve this as we are merging it to the swap-prevent-debt-cheques branch (relevant PR is #1983)
i want to see how this affects the incentives simulation tests, and we can also continue the discussion when we want to merge to master.
|
Superseded by #2051 |
This PR introduces a change in the sequence of message accounting.
Previously, when a message was marked as accounted via its pricing interface, it would be accounted for before sending.
This had negative side effects in that if sending a message crossed the payment threshold, a node would send the corresponding cheque first, which could bring a node into debt before the actual message would be arriving.
Related issues:
This PR changes the sequence, first the message is sent, and only if it succeeded, the message is accounted for.