Sorry for the long turnaround, great stuff, will try to merge it soonish - that is, after I finish, or perhaps pause, deploying big changes to load management...
On Tuesday 19 October 2010 22:57:50 Martin Nyhus wrote: > 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. We use it there too? Grrr! Somebody being way too clever for their own good! Anyway, thanks for fixing it! And yes, please fix it for old FNP too. Or bug me to do so. :) > > > 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. IMHO the packet length calculations for padding should be based on the full length. Are they? > > > - 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. Okay. > > > 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? Why would paddedLen *ever* be larger than maxPacketSize? I thought one of the purposes of this was to enable us to generate packets of almost any size? Granted it can be in old FNP, which is the origin of that code. > For now I > kept the clipping of that part (see 5f03804). This is fine. > > > 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 Great. Actually some of it is copied from various parts of our code, you should check that those files already have GPL2+ headers (I think they do..) > > > 177f81053b5636cd74ae4aece289221e41b4fb99 > > - Do we wake up the sender when we become unblocked? That's an important > > part of this too. > Done (6a9e0f8) Good. > > > 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. Hmmm. Calling your allocation method getSequenceNumber() is confusing, especially when packet has a similar method which is just a getter. It would help to change it to allocateSequenceNumber(). > > > > 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. IIRC the window is updated every time we reach half way into it? > > > > 2e33916fee9106b0e985f1bf974ed3d8fda40f42 > > > - Any particular reason for always starting from 0 on new format? > Done (807ea29) This starts with the same random sequence number in each direction afaics? That could be confusing. We should probably restrict the random range to not be too close to rollover. Especially as we need to be absolutely sure we rekey before we rollover, if necessary shutting down the connection to avoid sending two packets encrypted with the same sequence number. Do we? Also, we had originally planned to have a separate cipher key in each direction. This should be easy to implement now, and rather harder later on. > > > > 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) > Changes from 5e276c838e13440f28016ead6552bba80e07702e to e20d271a81a6520049e6658536fe4c43a7c387b8: - Fix javadocs/comments. - Synchronization fixes. - Fix clipping in padding rule (caused the maximum size to be more common than it needs to be). - Wake up the PacketSender when a peer's NewPacketFormat becomes unblocked (i.e. is able to allocate message ID's again) in processing an ack. - Clean up some code to make it more readable. - Start both packet and message seqnums at random numbers generated from the key setup. - Remove unnecessary code from unit tests. - Make a variable private. - Use a per-peer MersenneTwister to pad packets, rather than using the global fastWeakRandom (improves security). -------------- next part -------------- A non-text attachment was scrubbed... Name: signature.asc Type: application/pgp-signature Size: 197 bytes Desc: This is a digitally signed message part. URL: <https://emu.freenetproject.org/pipermail/devl/attachments/20101109/66bf41bb/attachment.pgp>
