On Saturday 16 October 2010 23:04:47 Matthew Toseland wrote: > 132f4b342068e2308d6ee27ad5d95ec82fb1be2e > - Please use /** Javadocs */ for the variables, Eclipse etc will take > advantage of them. Done (f10d41a)
> 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). Done (7c5a2bc) > 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. I'll have a look at MersenneTwister. Should it be used for FNP too (assuming it isn't too much work to change it)? If so I could probably fix it there as well. > 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 ... HMAC_LENGTH is the length of the HMAC. > - 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) The UDP overhead is already subtracted by UdpPacketSender, so maxPacketSize is the space we can use. > 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. How about when padding to the first multiple of 64 would be too much? For now I kept the clipping of that part (see 5f03804). > e2b0b8ee4e54b95839facdc66c781663e9f5626d > - Did you double check that this is all either exclusively your own code or > drawn from files that have the GPL2+ headers already? It is all my own code > 177f81053b5636cd74ae4aece289221e41b4fb99 > - Do we wake up the sender when we become unblocked? That's an important > part of this too. Done (6a9e0f8) > 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? AFACIS there are no paths where we would create a packet and then drop it. The sequence number is assigned at the very end of create packet, and we can't return null after that, and returning null is the only thing that would make us return before sending in maybeSendPacket. > > NewPacketFormat.tryDecipherPacket: > > - Might be clearer code if you moved the sequence number assignment out > > of the for(j), use continue outer instead of break. Done (abeb715) > > - 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? I don't see why it shouldn't, as only the sequence numbers that are outside the window are updated. > > 2e33916fee9106b0e985f1bf974ed3d8fda40f42 > > - Any particular reason for always starting from 0 on new format? Done (807ea29) > > 889a5cea3ed9b570f1f6e6784d34d61c9d932cd6 > > - Do we check for -1 in all getMessageID() callers? There is only one caller, and it is checked (NewPacketFormat:538). > > - Inconsistent synchronization in getMessageID()?? Fixed (f5f7d92) -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 198 bytes Desc: This is a digitally signed message part. URL: <https://emu.freenetproject.org/pipermail/devl/attachments/20101019/e8d5417f/attachment.pgp>
