-
Notifications
You must be signed in to change notification settings - Fork 22
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
gbn: Sender side only packeting resend bug fix #87
Merged
ellemouton
merged 9 commits into
lightninglabs:master
from
ViktorTigerstrom:2023-10-client-side-packeting-bug-fix
Jan 8, 2024
Merged
gbn: Sender side only packeting resend bug fix #87
ellemouton
merged 9 commits into
lightninglabs:master
from
ViktorTigerstrom:2023-10-client-side-packeting-bug-fix
Jan 8, 2024
Commits on Dec 1, 2023
-
Configuration menu - View commit details
-
Copy full SHA for c0eea25 - Browse repository at this point
Copy the full SHA c0eea25View commit details
Commits on Jan 8, 2024
-
gbn: change implication of processNACK signature
The processNACK function returns two booleans as its signature. The first boolean used to only indicate if the NACK was part of the queue, but is with this commit expanded to indicate if the queue needs to be resent or not as a result of the NACK. The second boolean indicates if the queue sequence base was bumped or not. Previously, the bumped value for processNACK didn't accurately reflect if the queue sequence base was actually bumped or not for the cases when we didn't trigger a resend. This commit addresses that issue. Updating the bumped value to be correct has no real impact on the current implementation of the gbn conn, but it might start having an effect if the implementation is modified in the future.
Configuration menu - View commit details
-
Copy full SHA for 6967ad3 - Browse repository at this point
Copy the full SHA 6967ad3View commit details -
Configuration menu - View commit details
-
Copy full SHA for 4c3528c - Browse repository at this point
Copy the full SHA 4c3528cView commit details -
The syncer struct can be used to ensure that both the sender and the receiver are in sync before the waitForSync function is completed. This ensures that we don't end up in a loop where one party keeps resending packets over and over again. This is done by awaiting that we receive either the an expected ACK or an expected NACK after resending the queue. The expected ACK is the ACK for the last packet that has been resent, and the expected NACK is the NACK sequence following the last packet that has been resent. To understand why we need to await the awaited ACK/NACK after resending the queue, consider the following scenario: 1. Alice sends packets 1, 2, 3 & 4 to Bob. 2. Bob receives packets 1, 2, 3 & 4, and sends back the respective ACKs. 3. Alice receives ACKs for packets 1 & 2, but due to latency the ACKs for packets 3 & 4 are delayed and aren't received until Alice resend timeout has passed, which leads to Alice resending packets 3 & 4. Alice will after that receive the delayed ACKs for packets 3 & 4, but will consider that as the ACKs for the resent packets, and not the original packets which they were actually sent for. If we didn't wait after resending the queue, Alice would then proceed to send more packets (5 & 6). 4. When Bob receives the resent packets 3 & 4, Bob will respond with NACK 5. Due to latency, the packets 5 & 6 that Alice sent in step (3) above will then be received by Bob, and be processed as the correct response to the NACK 5. Bob will after that await packet 7. 5. Alice will receive the NACK 5, and now resend packets 5 & 6. But as Bob is now awaiting packet 7, this send will lead to a NACK 7. But due to latency, if Alice doesn't wait resending the queue, Alice will proceed to send new packet(s) before receiving the NACK 7. 6. This resend loop would continue indefinitely, so we need to ensure that Alice waits after she has resent the queue, to ensure that she doesn't proceed to send new packets before she is sure that both parties are in sync. To ensure that we are in sync, after we have resent the queue, we will await that we either: 1. Receive a NACK for the sequence number succeeding the last packet in the resent queue i.e. in step (3) above, that would be NACK 5. OR 2. Receive an ACK for the last packet in the resent queue i.e. in step (3) above, that would be ACK 4. After we receive the expected ACK, we will then wait for the duration of the resend timeout before continuing. The reason why we wait for the resend timeout before continuing, is that the ACKs we are getting after a resend, could be delayed ACKs for the original packets we sent, and not ACKs for the resent packets. In step (3) above, the ACKs for packets 3 & 4 that Alice received were delayed ACKs for the original packets. If Alice would have immediately continued to send new packets (5 & 6) after receiving the ACK 4, she would have then received the NACK 5 from Bob which was the actual response to the resent queue. But as Alice had already continued to send packets 5 & 6 when receiving the NACK 5, the resend queue response to that NACK would cause the resend loop to continue indefinitely. When either of the 2 conditions above are met, we will consider both parties to be in sync, and we can proceed to send new packets.
Configuration menu - View commit details
-
Copy full SHA for 3b5967f - Browse repository at this point
Copy the full SHA 3b5967fView commit details -
Configuration menu - View commit details
-
Copy full SHA for 768ef7a - Browse repository at this point
Copy the full SHA 768ef7aView commit details -
Configuration menu - View commit details
-
Copy full SHA for 4a43139 - Browse repository at this point
Copy the full SHA 4a43139View commit details -
Configuration menu - View commit details
-
Copy full SHA for 6f969c9 - Browse repository at this point
Copy the full SHA 6f969c9View commit details -
Configuration menu - View commit details
-
Copy full SHA for 77d0bd5 - Browse repository at this point
Copy the full SHA 77d0bd5View commit details -
gbn+mailbox: force connection status change on FIN
When the client receives+processes a FIN packet, we force the connection to status ClientStatusSessionNotFound, as in rare occasions the corresponding server may have time to close the connection before we've already processed the sent FIN packet by the server. In that case, if we didn't force a new status, the client would never mark the connection as status ClientStatusSessionNotFound. This solves a bug for the testServerReconnect itest, as that test would sometimes fail due to the FIN packet being processed but without changing the connection status, leading to a test predicate never being satisfied.
Configuration menu - View commit details
-
Copy full SHA for 5e16121 - Browse repository at this point
Copy the full SHA 5e16121View commit details
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.