fix(overlay-service): Check for peer NodeId in discovery ENR cache
#935
Conversation
ogenev
force-pushed
the
fix-find-content-query
branch
from
a7af9ef to
31dd074
Compare
There was a problem hiding this comment.
Just a few questions so I can wrap my head around what's happening here, but it looks like a nice fix!
A thought I've had is... It seems like we're operating under the assumption that each NodeId correlates to a single network participant. Which is perfectly reasonable in theory. But in practice, we might have some (lazy) users using the same private keys (eg. 0x0000.., 0x1111.., 0x0101.., etc). While I don't think this is something we need to account for, it might be worth keeping in mind as we build these caching mechanisms. Though, I also feel like there's very little we can do about this case, besides simply encouraging users (via docs & defaults) to use truly random private keys if they want their node to run cleanly.
| @@ -264,6 +264,14 @@ impl Discovery { | |||
| self.node_addr_cache.write().get(node_id).cloned() | |||
| } | |||
|
|
|||
| /// Put a `NodeAddress` into cache. If the key already exists in the cache, then it updates the key's value and | |||
| /// returns the old value. Otherwise, `None` is returned. | |||
| pub fn put_cached_node_addr(&self, node_addr: NodeAddress) -> Option<NodeAddress> { | |||
There was a problem hiding this comment.
Looks like we're never calling this, besides in your test? Is there somewhere else in the production code that will update the node_addr_cache?
There was a problem hiding this comment.
Not sure about a use case to add a node to the cache outside of discovery, besides in testing. The other way around is to expose the cache and make it public but I think it is more foolproof to add a discovery method for it.
| service.discovery.put_cached_node_addr(node_addr); | ||
|
|
||
| let found_enr3 = service.find_enr(&node_id_3).unwrap(); | ||
| assert_eq!(found_enr3, enr3); |
There was a problem hiding this comment.
Should this test should include checking the case that the node id can still be found after the enr has been updated?
There was a problem hiding this comment.
I don't feel so, the scope of this test is to check if OverlayService find_enr is looking into the discovery cache. I think updating Enr is out of scope here.
Well, if someone cares about privacy and security, their private key should be random and secure. Operating a staking node or crypto wallet uses the same concepts so I don't think we will need to do a lot of teaching here. |
What was wrong?
Many
FindContentqueries fail when the result is an uTP stream. This is because we cannot find theENRof the sourceNodeIDand reject the incoming uTP content.This fix is expected to also improve
FindNodequeries.How was it fixed?
Check in the discovery ENR cache when trying to find an
ENRfor a knownNodeIdwhen processingFIndContentandFindNodequeries.To-Do