Looks like there's nothing really huge. If you can fix the remaining issues, that'd be great. We can merge it as soon as there is a window for a disruptive change (a couple of weeks probably, there are largish load management issues to deal with first). Thanks!
Up to 5e276c838e13440f28016ead6552bba80e07702e : - Simplify code slightly, remove unnecessary casts. - Fix off by ones in message ID handling. - If we have already received the message, ack it. - More unit tests, uncomment existing unit test. (Yay!) - Comments. - Don't use PacketTracker for sequence numbers. It's not appropriate, we don't want to block etc. - Support wrapping message numbers, various related changes. - Padding: Fragmented false and first fragment false means the rest of the message is padding. Add padding when the space allocated isn't filled. Pad with the same rule that current FNP uses. - Clearer defaults for some variables. - Logging. - Only use 31 bits for sequence number. - Return an accurate message length from addMessageFragment(). Prevents overestimating due to compression gaining us more than expected. - Check all possible matches from the watchlist. There can be more than one. - Call maybeRekey() when receiving a packet. - If we are unable to allocate a message ID in 10 minutes, throw a BlockedTooLongException. Add PacketFormat.canSend(), use it to ignore a peer if we can't send messages. Return true if we have nothing in progress and can't allocate a message ID because the message window is exhausted. Replaces existing wouldBlock() call for old FNP. Fixes busy-looping in this case. - Report incoming packets. - Explain unit test, add newlines to clarify. - Only use 2 bytes for message length and offset. - Move SentPacket creation to createPacket(). - Support adding multiple fragments of the same message to a packet. - Fall back to FNP if the packet cannot be handled by new packet format. - Remove unused variable. - Only track RTT on PeerNode, report round trips to PeerNode. 132f4b342068e2308d6ee27ad5d95ec82fb1be2e - Please use /** Javadocs */ for the variables, Eclipse etc will take advantage of them. aed60bea24a21db858ff784d95c3b9add2e99dad - You need to synchronize when setting highestReceivedSequenceNumber. Parallel decode of messages from the same peer is likely in the near future (nextgens is doing some optimisation work). f8ed9b4d4750a34a67c7fc3f0c2e3144b95cb838 - We should probably not use fastWeakRandom directly. It's entirely predictable. Probably we should create one MersenneTwister for each PeerNode and initialise it from the strong RNG. 7b2a66a803688e1be8d70f3e8eb3854f7b900242 - You should add and subtract the full overhead. Is HMAC_LENGTH the totality of the overhead apart from what's already accounted for in packet.getLength()? (I.e. the sequence number?) Looks like it is ... - Arguably if we are comparing to MTUs (or plausible MTUs such as 1280) we should count in the UDP/IP overhead as well? (28 bytes iirc) cd562aa4ebfb2a4c3b958efa8c4cd15fd7938d98 - Arguably we should truncate the parameter to nextInt() rather than randomising and then clipping; we don't do this in the old code, but it does create a pattern as well as using more entropy than needed. 12429efa8a5e0a014b83c1e28676733c395c37de - Are you planning to implement PMTU (or PMTU-like statistical guesswork e.g. track failures/successes by size and use it to compute that p(drop) is X% below N and Y% above it) or significantly reduce the default max packet size? 1280 including UDP/IP seems reasonable, or even a bit less; 576 is the minimum... e2b0b8ee4e54b95839facdc66c781663e9f5626d - Did you double check that this is all either exclusively your own code or drawn from files that have the GPL2+ headers already? 177f81053b5636cd74ae4aece289221e41b4fb99 - Do we wake up the sender when we become unblocked? That's an important part of this too. 9952ed2f7b5c658cc1287d8aec98471cbba0c7f0 - What happens if we create the packet then don't send it? Won't this waste a sequence number and thus cause bad things? General points: See other mail re sequence number wrapping. I agree with you about big packets and PMTU. Unfortunately, 1) PMTU in Java is very difficult (no way to set DontFragment flag), and 2) MTU under 1280 may not work anyway at least on opennet due to including noderefs in the auth packets. Would be interesting to look into this in future though... And it will work on darknet anyway. We should probably set the max packet size to 1280 for better compatibility? For this to achieve its promise and improve efficiency (as well as enabling transport plugins with relatively small packets and enabling support for low MTUs), we need a new padding rule. Either: a) We always send the largest possible packet. This will maximise bandwidth efficiency and probably minimise traffic analysis, at the cost of some latency, OR b) If there is high priority stuff (or urgent stuff??) to send, aggregate it, get the packet size, then (assuming it is not full), get the packet size, pad it, and then fill in the padding as much as possible with lower priority stuff. This should be configurable. Although I do wonder if the first strategy is better for most purposes... The current code more or less implements the first strategy, and because we can send big messages along with everything else, it should be more efficient than the old FNP, even though we have an extra few bytes per message... I've deleted what is definitely dealt with below. On Monday 16 August 2010 19:55:54 Matthew Toseland wrote: > > NewPacketFormat.tryDecipherPacket: > - Might be clearer code if you moved the sequence number assignment out of > the for(j), use continue outer instead of break. > - Should it be updated on every packet? Obviously it should never go > backwards... "it" here means the window I guess. Should it be updated incrementally i.e. on every packet? > > 2e33916fee9106b0e985f1bf974ed3d8fda40f42 > - Any particular reason for always starting from 0 on new format? > > 889a5cea3ed9b570f1f6e6784d34d61c9d932cd6 > - Do we check for -1 in all getMessageID() callers? > - Inconsistent synchronization in getMessageID()??
signature.asc
Description: This is a digitally signed message part.
_______________________________________________ Devl mailing list Devl@freenetproject.org http://freenetproject.org/cgi-bin/mailman/listinfo/devl
