AddNode in kad.RoutingTable should report status of addition attempt

[iand]

AddNode has the following signature:

AddNode(NodeID[K]) bool

It returns true if the node was added and false if it wasn't.

It would be useful to know why it wasn't added to distinguish between it being a duplicate and the routing table or bucket is full. Also some routing tables support adding a node to a pending list which are candidates to be promoted into a bucket if there is capacity at a later date.

I propose we change the signature to:

AddNode(NodeID[K]) AddNodeStatus

and define AddNodeStatus as:

type AddNodeStatus int

with the following values:

const (
    // AddNodeStatusUnknown is the default zero value for AddNodeStatus. It should not
    // be returned by AddNode.
    AddNodeStatusUnknown AddNodeStatus = 0

    // AddNodeStatusAdded indicates that the node was added to the routing table.
    AddNodeStatusAdded AddNodeStatus = 1

    // AddNodeStatusNotAdded indicates that the node was not added to the routing table 
    // but no specific reason is available.
    AddNodeStatusNotAdded AddNodeStatus = 2

    // AddNodeStatusNotAddedDuplicate indicates that the node was not added to the routing
    // table because it was already present.
    AddNodeStatusNotAddedDuplicate AddNodeStatus = 3

    // AddNodeStatusNotAddedNoCapacity indicates that the node would have been added to
    // the routing table but was not because there was not enough capacity. The caller could 
    // try again after removing another node from the table.
    AddNodeStatusNotAddedNoCapacity AddNodeStatus = 4

    // AddNodeStatusAddedPending indicates that the node was added to a pending list for
    // insertion into the routing table at a later time.
    AddNodeStatusAddedPending AddNodeStatus = 5
)

[dennis-tra]

At first, I thought this could be rather handled by separate error types. However, that wouldn't cover a case like AddedPending well 🤔

[iand]

An alternative design is the UsefulNewPeer method in go-libp2p-kbucket which analyses the suitability of the peer. However it doesn't report the reason for "usefulness" (though it could) and it is a racy operation, since the state of being useful changes quickly over time in a dynamic table.

[guillaumemichel]

Each routing table implementation can have its custom reasons to add or not to add a peer in a bucket. For instance, in the current IPFS DHT Routing Table a peer can be rejected because of the diversity filter. We can also imagine a Routing Table where a peer is rejected because its ping distance is too high etc. Hence IMO having an exhaustive list of AddNodeStatus doesn't really make sense.

If the caller of AddNode() wants to make use of the AddNodeStatus it must have the knowledge of the routing table implementation type (and not only that it implements the Routing.Table interface). As soon as the caller knows the type of the routing table, it could directly call a AddNodeCustom(NodeID[K]) CustomAddNodeStatus function that would be defined only for this routing table type. This specific function would be more precise in returning the exact reason why a peer was added or not.

AddNode(NodeID[K]) bool is simple enough that it can be implemented by all routing table implementations and can be used generically.

I would suggest that in the cases where the caller of AddNode needs to know more than a bool, a custom AddNodeX() AddNodeStatusX method is added in the routing table, and this method is used instead of the generic AddNode. WDYT?

[iand]

For instance, in the current IPFS DHT Routing Table a peer can be rejected because of the diversity filter. We can also imagine a Routing Table where a peer is rejected because its ping distance is too high etc. Hence IMO having an exhaustive list of AddNodeStatus doesn't really make sense.

Good points

Agree that routing tables can add this or an equivalent method and it's futile to try and enumerate all possible statuses. The caller can type assert to detect if the routing table supports an additional method that the caller can use.