p2p/discover: fix update logic in handleAddNode

[lightclient]

It seems the semantic differences between addFoundNode and addInboundNode were lost in
#29572. My understanding is addFoundNode is for a node you have not contacted directly
(and are unsure if is available) whereas addInboundNode is for adding nodes that have
contacted the local node and we can verify they are active.

handleAddNode seems to be the consolidation of those two methods, yet it bumps the node in
the bucket (updating it's IP addr) even if the node was not an inbound. This PR fixes
this. It wasn't originally caught in tests like TestTable_addSeenNode because the
manipulation of the node object actually modified the node value used by the test.

New logic is added to reject non-inbound updates unless the sequence number of the
(signed) ENR increases. Inbound updates, which are published by the updated node itself,
are always accepted. If an inbound update changes the endpoint, the node will be
revalidated on an expedited schedule.

[fjl]

It's better to store *enode.Node because that object is immutable. We just need to protect the access to the field node.Node.

[fjl]

Actually, I think this problem will go away with the refactoring where node is only used in table. The race happens because we return *nodesByDistance from Table.findnodeByID, which contains *node. When we access the inner node later, there is a race with the table updating it.

[fjl]

handleAddNode seems to be the consolidation of those two methods, yet it bumps the node in the bucket (updating it's IP addr) even if the node was not an inbound.

I don't think it's a problem. bumpInBucket is safe to invoke even for non-inbound nodes.

[lightclient]

Is it correct to update the IP of non inbound add? This cause the test TestTable_addSeenNode to fail.

[fjl]

I've thought about it a bunch more, and it used to be correct to not update the endpoint for found nodes, especially since the endpoint information is not authenticated in discv4. However, it's different in discv5 with ENRs. If we find a newer record, we want to put it into the table. I think we should resolve this by adding a check for the sequence number. When we encounter a newer record, update it, regardless of req.isInbound. As a special case we can also update the endpoint if req.isInbound && node.Seq() == 0 for discv4.

[fjl]

Pretty happy you looked into this because I didn't see these issues.

[Mazzika1]

Thank you

[lightclient]

Sure! I can update it.

[fjl]

Node refactoring is here #29844. It should fix the race, and this PR can just be about the handleAddNode thing.

[lightclient]

Updated.

[lightclient]

For discv5 the message has already been authenticated by now, correct? So that means only the actual node could double sign sequence 0?

Is it an issue that now in discv5 inbound contacts can update their endpoint by replaying sequence 0?

[fjl]

I don't think it's an issue. We could always be even stricter and check if it's an unsigned record. However, that seems excessive. We previously accepted all inbound updates, even ones with a lower sequence number, and in fact this is something I am still considering to add back. Endpoint information provided by the node itself can probably always be considered valid.

[fjl]

One thing I'm still exploring here is the interaction with revalidation. If a node changes endpoint via this update path, we should also move it to the fast reval queue again. This requires rethinking the queue management a bit.