To be clear, a TOO OLD/TOO NEW peer is currently connected in the sense
that the node is exchanging traffic with it. The node just doesn't
route any traffic through the peer and the traffic with the peer is
limited to keeping the connectivity, N2NMs and UoM exchanges (I don't
believe I'm forgetting anything, but it wouldn't be the first time).
Robert Hailey wrote:
> On Jan 2, 2008, at 3:44 PM, Matthew Toseland wrote:
>
>
>> On Wednesday 02 January 2008 17:22, Robert Hailey wrote:
>>
>>> On Dec 29, 2007, at 5:39 AM, Florent Daigni?re wrote:
>>>
>>>
>>>>>> synchronized void updateShouldDisconnectNow() {
>>>>>> //FIXME: We should not update VERIFIED unless we HANDSHAKE WITH
>>>>>> THE NODE
>>>>>> - if (isConnected()) {
>>>>>> + if (isConnected() || verifiedIncompatibleOlderVersion ||
>>>>>> verifiedIncompatibleNewerVersion) {
>>>>>> verifiedIncompatibleOlderVersion =
>>>>>> forwardInvalidVersion();
>>>>>> verifiedIncompatibleNewerVersion =
>>>>>> reverseInvalidVersion();
>>>>>> }
>>>>>>
>>>>> I suggest you call isRoutable() instead
>>>>>
>>>>> NextGen$
>>>>>
>>>> Hmm, on a latter thought, I don't understand why it helps... nor why
>>>> we
>>>> are calling it from PacketSender
>>>>
>>>> What about getting rid of both the "if" branch and the call in
>>>> PacketSender ?
>>>>
>>> In PacketSender this function is called ~precisely~ at the version
>>> transition time so that all peers will be re-evaluated concerning
>>> compatible versioning; this seems like an odd place to stick this
>>> code, but also updateShouldDisconnectNow() does not explicitly
>>> disconnect the peer node, it only updates the 'incompatible' flags.
>>>
>> We don't want to disconnect. We want to stay connected and to not
>> route
>> requests to the node or accept them from it. We will still accept
>> e.g. Update
>> Over Mandatory traffic.
>>
>
> Fair enough, but this is only confused semantics. I said disconnect as
> it is in the function name, perhaps a better name would be
> updateVersionRoutablity().
>
>
>>> Concerning the if branch I added, the intended behaviour change is
>>> that this function should only (1) allow the disconnection of a
>>> connected peer by setting the incompatible flag,
>>>
>> No, we don't want to disconnect. We just want to make it non-
>> routable, by
>> updating verified*.
>>
>
> I used the wrong term. It will only update the verified* booleans (and
> thus make routable or not) if connected. But see below...
>
>
>>> Now, if the nodes can handshake, this code has probably has little
>>> effect, as it simply further restricts one of only two places that
>>> the
>>> incompatible flags are set (the other being on a handshake). What it
>>> will do is if the nodes can't talk to each other (like in my re-keyed
>>> node case), is not show 'TOO_OLD'/'TOO_NEW', but instead
>>> 'DISCONNECTED'.
>>>
>> So you are setting verified* to false, because it's not verified?
>>
>
> Not setting it to false, but... not allowing it to be set to true if
> that node cannot reach us (e.g. disconnected).
>
>
>>> And I suppose in the same error case... if the node is
>>> restarted, it would never be able to verify that the peer is too old/
>>> new and fetch the ark (keeping the version number up-to-date).
>>>
>> Again, we should always fetch the ARK.
>>
>
> The original implementation appears to use verifiedIncompatible* to
> optimize against a time-based trigger (Version-Incompatibility) from a
> bunch of ark requests being started.
>
> On Jan 2, 2008, at 12:30 PM, David Sowder wrote:
>
>> I'm not sure of the thinking of others regarding changes to the
>> verifiedIncompatible variables since, but my thinking in my initial
>> implementation was that they would only be set after a handshake, thus
>> we already had a connected to them and ARKs weren't needed. That
>> won't
>> change until the peer re-handshakes after restarting with a different
>> version of the node's software.
>>
>
> As best I can see, the verified* booleans have changed meanings since
> update-over-mandatory. If it is still useful to know if the peer node
> is presently-verified-{older/newer}, then we should split the booleans
> (isOlder,isVerifiedOlder); otherwise just fetch an ark even if it is
> an older (as Matthew suggested). In either case the misleading
> comments and variable names should be removed and/or changed.
>
> --
> Robert Hailey
>
> _______________________________________________
> Devl mailing list
> Devl at freenetproject.org
> http://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
>
