From abb8cbe75d02d16fd8c764bf6b97e9f1a560bd36 Mon Sep 17 00:00:00 2001
From: zzz <zzz@i2pmail.org>
Date: Mon, 21 Dec 2020 10:02:20 -0500
Subject: [PATCH] SSU: Fix partial acks not being sent when there are no
'gaps'. Workaround the bug on the sending side for pre-0.9.49 routers by
sending fragments in reverse order.
Bug introduced in commit 9c4558d8919bf58a50a8715b6e5605a21f95bab5 Sep 20 2014.
This partially reverts that commit.
Reported by and adapted from a patch by zlatinb
---
.../transport/udp/InboundMessageState.java | 22 ++++++++++++++++---
.../transport/udp/OutboundMessageState.java | 11 ++++++++--
.../router/transport/udp/PacketBuilder.java | 2 --
.../i2p/router/transport/udp/PeerState.java | 3 +--
4 files changed, 29 insertions(+), 9 deletions(-)
diff --git a/router/java/src/net/i2p/router/transport/udp/InboundMessageState.java b/router/java/src/net/i2p/router/transport/udp/InboundMessageState.java
index f2209674f7..15a255aad0 100644
--- a/router/java/src/net/i2p/router/transport/udp/InboundMessageState.java
+++ b/router/java/src/net/i2p/router/transport/udp/InboundMessageState.java
@@ -226,7 +226,11 @@ class InboundMessageState implements CDQEntry {
return _completeSize;
}
- /** FIXME synch here or PeerState.fetchPartialACKs() */
+ /**
+ * Only call this if not complete.
+ * TODO remove this, have InboundMessageState implement ACKBitfield.
+ * FIXME synch here or PeerState.fetchPartialACKs()
+ */
public ACKBitfield createACKBitfield() {
int last = _lastFragment;
int sz = (last >= 0) ? last + 1 : _fragments.length;
@@ -236,9 +240,12 @@ class InboundMessageState implements CDQEntry {
/**
* A true partial bitfield that is probably not complete.
* fragmentCount() will return 64 if unknown.
+ *
+ * TODO remove this, have InboundMessageState implement ACKBitfield.
*/
private static final class PartialBitfield implements ACKBitfield {
private final long _bitfieldMessageId;
+ private final int _fragmentCount;
private final int _ackCount;
private final int _highestReceived;
// bitfield, 1 for acked
@@ -263,6 +270,7 @@ class InboundMessageState implements CDQEntry {
}
}
_fragmentAcks = acks;
+ _fragmentCount = size;
_ackCount = ackCount;
_highestReceived = highestReceived;
}
@@ -274,7 +282,12 @@ class InboundMessageState implements CDQEntry {
return 1L << fragment;
}
- public int fragmentCount() { return _highestReceived + 1; }
+ /**
+ * Don't use for serialization since it may be unknown;
+ * use highestReceived() instead.
+ * @return 64 if unknown
+ */
+ public int fragmentCount() { return _fragmentCount; }
public int ackCount() { return _ackCount; }
@@ -288,7 +301,10 @@ class InboundMessageState implements CDQEntry {
return (_fragmentAcks & mask(fragmentNum)) != 0;
}
- public boolean receivedComplete() { return _ackCount == _highestReceived + 1; }
+ /**
+ * @return should always be false
+ */
+ public boolean receivedComplete() { return _ackCount == _fragmentCount; }
@Override
public String toString() {
diff --git a/router/java/src/net/i2p/router/transport/udp/OutboundMessageState.java b/router/java/src/net/i2p/router/transport/udp/OutboundMessageState.java
index 4d76978e1f..15401304dc 100644
--- a/router/java/src/net/i2p/router/transport/udp/OutboundMessageState.java
+++ b/router/java/src/net/i2p/router/transport/udp/OutboundMessageState.java
@@ -259,7 +259,8 @@ class OutboundMessageState implements CDPQEntry {
int minSendCount = getMinSendCount();
// Allow only the fragments we've sent the least
int rv = 0;
- for (int i = 0; i < _numFragments; i++) {
+ // see below for why we go in reverse order
+ for (int i = _numFragments - 1; i >= 0; i--) {
if (needsSending(i) && _fragmentSends[i] == minSendCount) {
int sz = fragmentSize(i) + overhead;
int tot = rv + sz;
@@ -344,7 +345,13 @@ class OutboundMessageState implements CDPQEntry {
int minSendCount = getMinSendCount();
int sent = 0;
int overhead = _peer.fragmentOverhead();
- for (int i = 0; i < _numFragments; i++) {
+ // Send in reverse order,
+ // receiver bug workaround.
+ // Through 0.9.48, InboundMessageState.PartialBitfield.isComplete() would return true
+ // if consecutive fragments were complete, and we would not receive partial acks.
+ // Also, if we send the last fragment first, receiver can be more efficient
+ // because it knows the size.
+ for (int i = _numFragments - 1; i >= 0; i--) {
if (needsSending(i)) {
int count = _fragmentSends[i];
if (count == minSendCount) {
diff --git a/router/java/src/net/i2p/router/transport/udp/PacketBuilder.java b/router/java/src/net/i2p/router/transport/udp/PacketBuilder.java
index d6d9984fc6..f8eba5b786 100644
--- a/router/java/src/net/i2p/router/transport/udp/PacketBuilder.java
+++ b/router/java/src/net/i2p/router/transport/udp/PacketBuilder.java
@@ -364,8 +364,6 @@ class PacketBuilder {
for (ACKBitfield bf : partialACKsRemaining) {
if (partialAcksToSend >= ABSOLUTE_MAX_ACKS)
break; // ack count
- if (bf.receivedComplete())
- continue;
// only send what we have to
//int acksz = 4 + (bf.fragmentCount() / 7) + 1;
int bits = bf.highestReceived() + 1;
diff --git a/router/java/src/net/i2p/router/transport/udp/PeerState.java b/router/java/src/net/i2p/router/transport/udp/PeerState.java
index bdeb8d0b93..ae604a05b6 100644
--- a/router/java/src/net/i2p/router/transport/udp/PeerState.java
+++ b/router/java/src/net/i2p/router/transport/udp/PeerState.java
@@ -992,8 +992,7 @@ public class PeerState {
if (states != null) {
for (InboundMessageState ims : states) {
ACKBitfield abf = ims.createACKBitfield();
- if (!abf.receivedComplete())
- rv.add(abf);
+ rv.add(abf);
}
}
}
--
GitLab