Torpush gossip- creating separate tor based gossip sub instance for broadcasting over tor #3
Conversation
| @@ -2616,7 +2686,8 @@ proc broadcastAttestation*( | |||
| let | |||
| forkPrefix = node.forkDigestAtEpoch(node.getWallEpoch) | |||
| topic = getAttestationTopic(forkPrefix, subnet_id) | |||
| node.broadcast(topic, attestation) | |||
| echo "about to send attestion" | |||
| node.broadcastTor(topic, attestation) | |||
There was a problem hiding this comment.
The main concern I'd have with relaying on a non-just-broadcast codepath is that it's too easy to accidentally leak (meta)data by leaving in place some node.broadcast() somewhere. So probably even for an experimental approach, it would be better to by any of various means ensure either
- none of these
node.broadcast()s ever get called, or - it automatically does the Tor broadcast, not non-Tor broadcast.
Tor and non-Tor broadcast co-existing seems risky.
There was a problem hiding this comment.
How about moving procs like broadcastAttestation and the ones that need broadcastTor to another file and keeping broadcastTor private? This file might benefit from some reorganization for readability and maintainability.
There was a problem hiding this comment.
The coexistence of these tor based calls and normal calls in the code would be refactored to avoid leakage risk you mentioned. The concern is that for Blocks/attestatations/proofs/slashing/discovery etc.. should we move all over Tor if one of them is being transmitted over Tor? This needs to be examined further. As for code refactoring improvements, we can make tor methods private or in a separate file.
There was a problem hiding this comment.
Probably, moving most of them is required if any of them are moved. Ideally, yes, it's either all Tor or not at all, but it might be reasonable to avoid Tor for the largest and least frequent messages, such as blocks.
But attestations, discovery, et cetera all happen so regularly that it'd be easy to construct a correlation attack along the lines of #3 (comment), yes.
| @@ -1290,7 +1297,7 @@ proc checkPeer(node: Eth2Node, peerAddr: PeerAddr): bool = | |||
| else: | |||
| true | |||
|
|
|||
| proc dialPeer(node: Eth2Node, peerAddr: PeerAddr, index = 0) {.async.} = | |||
| proc dialPeer(node: Eth2Node, peerAddr: PeerAddr, index = 0, isnotTor = true ) {.async.} = | |||
There was a problem hiding this comment.
Basically same comment as the broadcastTor and broadcast coexisting: this seems like an auditing challenge, to make sure that a function with a security-important optional parameter is always called correctly.
There was a problem hiding this comment.
Maybe we can do something like that:
proc dialPeer(node: Eth2Node, peerAddr: PeerAddr, index = 0) {.async.} =
await internalDialPeer(node, peerAddr, node.switch)
proc dialPeerThroughTor(node: Eth2Node, peerAddr: PeerAddr, index = 0) {.async.} =
await internalDialPeer(node, peerAddr, node.torSwitch)
There was a problem hiding this comment.
Okay that can be separately called. That would be part of other refactoring suggested.
|
Ready for integration in main experimental branch |
|
Merging pull request |
This pull requests provides the first tor based gossipsub instance usage with in nimbus to broadcast attestations over tor.
The validator was successfully run based on this and stats were collected
Stats collected
Validator