Florent Daignière wrote:
> * Robert Hailey <[EMAIL PROTECTED]> [2008-01-02 18:23:28]:
>
>
>> 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.
>>
>
> We want to fetch ARKs in any case imo.
>
And in my opinion, fetching an ARK for a connected peer is pointless,
but perhaps we agree in our assessments and I misunderstand by thinking
you mean to _always_ fetch ARKs regardless of current peer status.
_______________________________________________
Devl mailing list
Devl@freenetproject.org
http://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
