Separating tor switch context from normal for better privacy #4
Conversation
…h normal adding tor connect handling
ujscale
requested review from
kaiserd,
diegomrsantos,
Menduist and
ufarooqstatus
|
Succuessful run log: __ is a tor switch `connect successful, Switch type:(peerId: 16Uiu2HAky12PxRUE2w75yvp6CAAV6KqGLLYSZ8ivGYrHYAX4xwgt, addrs: @[/ip4/54.90.199.254/tcp/9000]) |
| true | ||
| # check if the normal switch is already using the same connection | ||
| # avoid any overlap with normal peer use to prevent leakage or correlation | ||
| if node.torpeerPool.hasPeer(peerId) : |
There was a problem hiding this comment.
I'm not sure this is a good idea, this could actually cause a correlation
There was a problem hiding this comment.
The point is that both normal switch and tor switch are looking for attestation topic subnet peers! So we should have a way to avoid overlap as the attacker can build a correlation of active normal connections from peer with the concurrent connections through tor within the same time period! So that's why torswitch will check if normal is already connected with a peer that it should avoid making connection to and viceversa.
Ah, there could be a better way. Why not just keep attesstion_net peers specifc for torswitchh only. So the normal switch should not find attestation subnet peers at all or make any connections to them !
|
The code duplication isn't great here, but that should work for a PoC |
Well, there is a better way either to inherit from Eth2Node with a subclass TorEth2Node subclass that reuses all these functions or through template that use node switch typevariable. This refactoring could be done after we get new behaviours tested. |
|
@tersec adding tersec |
ujscale
requested review from
Menduist and
Terrytobin
and removed request for
Terrytobin
|
See if its possible to add tersec and zahary to this. |
| @@ -71,19 +71,24 @@ type | |||
| wantedPeers*: int | |||
| hardMaxPeers*: int | |||
| peerPool*: PeerPool[Peer, PeerId] | |||
| torpeerPool*:PeerPool[Peer, PeerId] | |||
There was a problem hiding this comment.
Generic, as it were, comment relating to all of these foo and torfoo pairs: they create code duplication, which can be avoided by having an entire Tor switch, so it's be torSwitch.peerPool, etc.
As usual, it depends what the goals here are, how they're balanced between research and deployability.
But more or less all the remaining hundreds of lines of code changes here are duplications of functions resulting from this design decision.
proc torcheckPeer(node: Eth2Node, peerAddr: PeerAddr): bool =
logScope: peer = peerAddr.peerId
let peerId = peerAddr.peerId
if node.torpeerPool.hasPeer(peerId):
trace "Already connected over tor"
false
else:
if node.istorSeen(peerId):
trace "Recently connected over tor"
false
else:
# check if the normal switch is already using the same connection
# avoid any overlap with normal peer use to prevent leakage or correlation
if node.peerPool.hasPeer(peerId) :
echo "Avoiding Overlap of tor with normal's ", peerAddr
false
else:
trueisn't meaningfully different from checkPeer except that it refers to torpeerPool, rather than peerPool, and calls istorSeen rather than isSeen.
There's some notion of that it has to know about the peerPool too, but in general, there are more precise ways to satisfy most of these constraints.
So this is the question: as research code, sure, why not.
I'd push against incorporating this design into actual-nimbus-eth2, if that were proposed.
| declareGauge nbc_torgossipsub_healthy_fanout, | ||
| "numbers of topics over tor with dHigh fanout" | ||
|
|
||
|
|
There was a problem hiding this comment.
Extra newline? Doesn't really matter, maybe intentional (group declareGauges, etc), but wasn't there before in that capacity.
Trivial nit, sure.
+ script fix
Using tor for all validator broadcast type
This PR aims at minimizing the overlap of torswitch with the context of normal switch by separating peerpool, connection handling, queues, discoveryloop and attestation net finding.
#5