-
Notifications
You must be signed in to change notification settings - Fork 378
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Remove dependency of non-protocol required confirmation messages (Delivered, Processed) #3254
Comments
On last bullet point, about |
We have discussed this in the past multiple times, in my opinion it is a bad idea. My comments on the proposal: Regarding the abstract
I believe the claim about latency here is wrong (I agree with the load though). From the perspective of a single transfer: There is no waiting involved with either Delivered or Processed messages. These messages do not contain a balance proof, and therefore don't need to be ordered, meaning they can be sent as soon as it is appropriate ( == no waiting/latency). Additionally, these messages are sent backwards in respect to the protocol messages, to a different recipient, meaning these messages are sent in parallel to the protocol messages. From the perspective of a single node: In this case, I actually think your proposal will add latency instead of removing it. Assume that you have a channel A-B, and that node A is trying to send two transfers through this channel at the same time, now assume the first transfer has a longer path than the second. Assume that messages are sent one at the time, then the second transfer will have to wait for the longer path to complete its protocol before proceeding with the second, increasing the latency. This point generalizes and holds even if messages are batched, the difference is that instead of talking about a message we would have to talk about a group of messages.
This is true for the current implementation, however, signing is being used to authenticate the messages to prevent an attack. The important thing here is that we can use different mechanisms to do the authentication, e.g. end-to-end encryption, with E2E user intervention isn't necessary, as long as the channel is not compromised (e.g. the symmetrical key is safe). Regarding the motivation
I agree completely with this statements here. Yes, these two messages have a unique purpose. Delivered messages inform the sender that the recipient has received and safely stored the message. Processed messages means that the message was not only stored, but also that the contents of the message which had to be validated against the blockchain were validated and accepted as valid. I not only think that having a single meaning per message is fine, I also think it's something we should really strive for. Regarding the specification
You are assuming that either a secret request or a refund transfer will always arrive, what if neither arrive? This could happen for a variety of reasons:
This is a good example of why having multiple responsibilities for a single message can be bad, what if the secret is learned by the mediator through the blockchain? Because the node learned it from the blockchain it knows that everyone else in the path learned it from the blockchain, including the previous mediator and the initiator, and for the current version of the code base it does not send a secret reveal backwards, it's only done when the secret is learned off-chain The proposed change would require us to review all messages and code paths to ensure that they are sent for the unhappy case too, otherwise the node will be unable to clear the message from the queue and make progress.
This can actually deadlock if messages are sent one at the time and there is a refund transfers. Consider the following path and order messages in the queue: A-B-C-B-D Batching could fix the above example, however, we would have to introduce a requirement for the minimum batch size to the transport layer and assume this size ensures progress when the queue is bigger than it. |
Thank you, @hackaugusto for your thoughts. But I think some misunderstandings happened, and would like to get your position on the clarifications below:
That's true for UDP, but on matrix you need to consider each event incurrs in ramp-up-down times for
Why? My idea is that the queues won't be halted by a message not yet confirmed, but instead, all messages should be sent as soon as they arrive (and/or batched), and protocol will always move forward regardless of confirmation method. This proposal is about changing the machanics of when we stop retrying some message being sent, but not requiring this to happen at all for the protocol (including different transfers going through the same channel) to go on.
Agree, this point specifically can also be replaced by transport features. Nonetheless, IMO the other points still hold and we can shrink the protocol and needed roundtrips through this proposal.
Yes, I know they mean different things, but my proposal is that we can drop this distinction, and implement the actual effect of these messages (in this case, stopping the retry) implicitly through the actual protocol messages, instead of specific ones.
This is built-in. I considered that any of these messages could never arrive, in which case (via this proposal) per protocol they should then be retried until the message is not valid anymore (in this case, lock expiring). If the situation change, the message can eventually be accepted and transfer go through. If not, and transfer never finish, they should continue to be retried until the lock expires or the channel is closed, making them invalid and retry stopped.
This only affects the condition in which the message (in this case,
That's not needed. As I said, the messages can be batched, but batching is just an optimization, they're always sent anyway (preferably in-order, but not necessarily, that's why we rely on the retry mechanism). They won't deadlock, as all messages will always be sent, and eventually should be accepted, or else continue retrying until some higher-tier condition is invalidated (e.g. channel closed). This proposal makes the delivery/acceptance of the messages async with the protocol. Once the state moves on sender (and then a respective message is queued to partner), sender can move on, making new state changes and queueing other messages on top of it. Eventually, they should be accepted through retry mechanism, and once the side effect of it is detected, we can give up retrying the message. Until then, everything is async, and we can move forward in the protocol. |
This is not specific to the delivered and processes messages, how does this implies these messages incur latency?
The above assumes that only the nth-first messages of the queue are sent.
That was the point, implicit meaning is not a good thing.
This is not a sufficient condition, what if the recipient is offline while the lock expires? The locked transfer retries stop and the remove expire lock is added to the queue, but remove expire lock will never be processed without the locked transfer.
I know that we can try to figure out what are all conditions for retries, that is not the point. The point is that this is extra work, more error prone, and difficult.
Again, I'm assuming the first nth-messages are sent. Your batch is the whole queue.
So, from what I can see the proposal is to:
For 1. I think e2e is a better solution. for 2. I'm not convinced it will have a positive impact. For 3 it's probably negligible, delivered and processed messages with a signature are around 77 bytes, without it they are 12 bytes. |
incur latency because they're unneeded. any unneeded message incur latency, given matrix internals
All messages are always retried, until we're certain they can be cleared. If an out-of-order message is rejected, their previous message will be retried, and eventually accepted. "queue" won't be halted by non-confirmed messages.
IMHO it is, if it's inherent from the protocol. e.g.
You're right, then keep sending them until the
I think it's not more difficult than having specific messages with specific state changes and conditions handled to do the same, which we need to keep in sync and ensure does what it's expected.
This whole proposal has nothing to do with matrix's persistency, nor requires it, it was dropped in favor of transport-agnostic retry-ordering mechanism.
Yes, it is, but is doesn't need to be, messages are retried independently of each other, and will eventually be accepted in order. So, for 1. e2e may tie us to the transport, I'd rather not. Latency and load are not the main reasons for this proposal, but a side effect of the actual target: reduced number of messages required for the core protocol. IMHO latency and load are just a couple of the number of ways a protocol with just the bare minimum size is better than a fatter one. |
In Monday's transport teem meeting we decided on a deadline for discussion on this issue no later than 2019-01-25. |
My take away from this discussion: The overall discussion is whether we should remove the dependency on receiving From one hand, if i take the example:
What could happen is that either the On the other hand, the same example of:
Could present different questions. Such as: What if the secret was registered on-chain. Since the transport is agnostic about whats happening on the blockchain, this could result in the LockedTransfer to be retried multiple times although the secret became known. The way to stop this message from being retried is to either have the transport also check blockchain events or for the state machine to inform the transport about this which is a bad design (the current architecture implements the complete opposite). One more example can be given in a topology of channel A-B where A sends a locked transfer to B, B processes that message and goes offline. Later on, A expires the lock, the lock expiry will never be sent because the locked transfer was not removed from the queue because no message was received by A from B saying that this message was processed. Therefore, protocol was halted. My opinion: i agree with Augusto on the point:
IMO the To be discussed and decided on. |
@rakanalh good points. Yes, your understandings are mostly right, only some clarifications:
The second case is actually the current implementation, and the intended one: core/state machine knows when a message was received/processed (through handling of the confirmation messages), and push to the transport the information it can give up retrying that message. The only observation is that it does so implictly, by removing the message from the queue, which is monitored by transport to give up (#3252 contains a proposal to make that explicit as well). This case you pointed out is an edge case of the one raised by Augusto, in which the to-be expected side effects of the LockedTransfer doesn't happen, but the LockExpired which would be the next one to stop retry of the LockedTransfer isn't sent either (because the secret was revealed, so the lock didn't actually expire).
Not true, in current implementation and also on this proposal, messages continue to be retried until they're cleared. LockExpired doesn't depend on LockedTransfer to be cleared from queue to be sent. All messages are retried, and ordered both on sender and (as per this proposal) on receiver as well. LockedTransfer will be eventually accepted, and LockExpired as well, which Processed will confirm both were accepted and clear them on sender. A retrying message doens't lock the queue, they're always retried in order and eventually accepted and cleared. |
You're right, currently they're part of the protocol. This proposal is about arguing they aren't strictly needed for it, somehow superfluous, and could be (mostly) taken out of it (at least as a hard requirement of the protocol, could be kept for compatibility for some time). Again, protocol is what we say it is. Granularity of messages is subjective, about what we consider different meanings for a given message, depends a lot on perspective. If looking closer enough, most messages has multiple meanings, and if zooming out, we could merge some of them to reduce protocol size. |
If this "side effect instead of Processed" isn't accepted, a compromise would be to use this approach at least for getting rid of |
I'm working on a small PoC for the |
On the workshop, it was decided on the following approach for now:
|
Note: this is just to make it work with the current code base, it should however not be a retried message. See also raiden-network#3254 (comment)
Note: this is just to make it work with the current code base, it should however not be a retried message. See also raiden-network#3254 (comment)
Issue was edited to include current proposal, after talks with @hackaugusto . This shouldn't take too long, but could break compatibility (it can be kept with a little more work though, although we could skip keeping it for now and just asking clients to upgrade). |
Just to be clear, my opinion is written below and it includes only the things written bellow, nothing else:
And the bugs above should be fixed. We have a few ways to fix this:
Edit: Important note: The processed message is being added to the global queue, meaning that it does not hold off balance proof messages. |
Note: this is just to make it work with the current code base, it should however not be a retried message. See also raiden-network#3254 (comment)
Note: this is just to make it work with the current code base, it should however not be a retried message. See also raiden-network#3254 (comment)
Note: this is just to make it work with the current code base, it should however not be a retried message. See also raiden-network#3254 (comment)
Note: this is just to make it work with the current code base, it should however not be a retried message. See also raiden-network#3254 (comment)
Note: this is just to make it work with the current code base, it should however not be a retried message. See also raiden-network#3254 (comment)
This was done in the Light Client, and in a backwards compatible manner.
Delivered messages are superfluous on this setup, but still sent for Processed, SecretRequest, SecretReveal iff the peer doesn't advertise |
#5581 partially resolves the issue. What it's doing can be read there.
Breaking backward compatibility would make the codebase easier. After the RC and release we should have to stay with the protocol. |
have a call with all stakeholders to decide the best approach. |
#3283 (comment) I describe how we implemented Capabilities. If you guys decide to implement it, you can worry less about breaking compatibility as protocol differences can be negotiated through them, as long as thenode can fallback to previous behavior. |
Results from the call:
|
@hackaugusto We do send |
So #5581 will be part of Alderaan, whereas further optimization described in this issue are not part of it. |
Ok, we're revisiting what was left for this on the LC for raiden-network/light-client#1514 and raiden-network/light-client#249 . |
Basic capability publishing support as been implemented via #6482. |
General approach
ToDo
|
|
Abstract
Raiden protocol requires strong guarantees about delivery and acceptance (correctly processing) of most of its messages, due to strict requirements related to state consistency between parts. Initially because of limitations of the UDP protocol, Raiden added some confirmation-control messages which are not specific to the inner working of the protocol, but are required specifically [only] to control retry of the message, until they're received/acccepted or some breaking event happens (e.g. channel is closed, no need to continue retrying).
Currently, the implementation is that
sender
should keep retrying sending any message until they receive a respectiveDelivered
orProcessed
message, indicating it was correctly received or accepted by the partner (see #3087). This adds load and latency to the transport which isn't actually required for proper working of the protocol, and also requires additional processing due to signing and validation of every message, specially problematic if one have interactive signing (possibly the case for a future light client implementation, #114) or limited hardware (#3199 , #2385).Motivation (iteration 1)
In talks with @hackaugusto , it became clearer that
Delivered
andProcessed
were split from anAck
message on UDP times, which had mostly the function of currentProcessed
message, which is to confirm correct processing and stop retry of a given message, currently replied toBalanceProof
-containing messages. For that, we may removeDelivered
messages completely, as most of its function is already provided byProcessed
messages.Specification
Currently,
Delivered
messages are used to confirm 3 messages:SecretRequest
,SecretReveal
andProcessed
, and this is how we can remove the need for it:SecretRequest
: simplest case, confirmed by the respectiveSecretReveal
SecretReveal
: although doesn't contain a balance-proof, it can be confirmed by aProcessed
message, as its content needs to be validated against the state (e.g. there's a pending lock with the given secret), so aProcessed
message should confirm it instead ofDelivered
(which means nothing on this context anyway, effectively replacing Delivered for Processed for SecretReveal).Processed
: become a one-shot message, as currently isDelivered
, meaning it isn't retried, but sent only once as a response to some other message. One-shot messages doesn't need confirmation (therefore, noDelivered
required), but also needs to handle duplicate/retried messages coming.nonce
is smaller than current one, a new (one-shot, even if )Processed
message is generated and sent anyway.SecretReveal
message, as proposed above, if there's a pending or unlocked lock with the given hash, aProcessed
should be sent to confirm itThis proposal also supersedes #2383 and #2505 . May be related to #490 (as per receiver by-nonce queue and caching of responses).
Backwards Compatibility
The
Delivered
messages could still be sent for backwards compatibility, and some handling of it could still be done for theSecretReveal
confirmation coming from old clients. Nonetheless, this could be a good opportunity to improve protocol by breaking compatibility, possibly also doing #3283 before next release.Timeboxing
Investigation and split into more detailed issues or implementation timeboxed to 2 PD
The text was updated successfully, but these errors were encountered: