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
