-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Conversation
Ideally the new CLI options would only be added to the kitchen-sink node, but the best way to achieve this is not obvious to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had a few question from my first reading, at this point I did not yet get the session even odd logic, but will look at it soon.
@@ -165,6 +168,7 @@ impl<T: offchain::DbExternalities + Clone + Sync + Send + 'static> DbExternaliti | |||
pub struct ExecutionExtensions<Block: BlockT> { | |||
strategies: ExecutionStrategies, | |||
keystore: Option<KeystorePtr>, | |||
mixnet_kx_public_store: Option<Arc<MixnetKxPublicStore>>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thinking out loud (no idea if better), could this have been part of existing keystore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think the requirements overlapped enough for this to make sense. The existing keystore is (AFAICT) designed for long-lived keys that persist across node restarts. The mixnet key-exchange keys are only used for a single session (a few hours), and should be destroyed once the session ends. They intentionally do not persist across node restarts, as this would be non-trivial to implement properly:
- When destroying keys you would have to be careful to properly erase the key data from disk.
- The corresponding replay filters would also have to persist, or you would be vulnerable to replay attacks after a node restart.
I opened a GitHub issue about this a while back (#12595), there wasn't really any conclusion though.
frame/mixnet/src/lib.rs
Outdated
|
||
impl<MaxPeerIdSize, MaxExternalAddressSize, MaxExternalAddresses> Into<OpaqueMixnode> | ||
for BoundedOpaqueMixnode< | ||
BoundedVec<u8, MaxPeerIdSize>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure if doable but type aliasing BoundedVec<u8, MaxPeerIdSize>
to PeerIdRuntime or a better name would be good.
Also for BoundedVec<u8, MaxExternalAddressSize>
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would like to write something like impl<T: Config> Into<OpaqueMixnode> for BoundedOpaqueMixnodeFor<T>
here, but the Rust compiler doesn't like this:
error[E0207]: the type parameter `T` is not constrained by the impl trait, self type, or predicates
--> frame/mixnet/src/lib.rs:84:6
|
84 | impl<T: Config> Into<OpaqueMixnode> for BoundedOpaqueMixnodeFor<T>
| ^ unconstrained type parameter
I can do something like this, but not sure it's better:
pub type BoundedPeerId<MaxSize> = BoundedVec<u8, MaxSize>;
pub type BoundedExternalAddress<MaxSize> = BoundedVec<u8, MaxSize>;
pub type BoundedExternalAddresses<MaxSizeEach, Max> =
BoundedVec<BoundedExternalAddress<MaxSizeEach>, Max>;
impl<MaxPeerIdSize, MaxExternalAddressSize, MaxExternalAddresses> Into<OpaqueMixnode>
for BoundedOpaqueMixnode<
BoundedPeerId<MaxPeerIdSize>,
BoundedExternalAddresses<MaxExternalAddressSize, MaxExternalAddresses>,
>
|
||
#[pallet::call] | ||
impl<T: Config> Pallet<T> { | ||
/// Register a mixnode for the following session. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is there a way to register a change of external address for the current session?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. Doesn't feel worthwhile to implement this to me; sessions should only be a few hours?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if a few hour offline is acceptable (probably is initially).
Is there (or is there going to be) some kind of documentation for how this works at the protocol level somewhere? (source code doesn't count, since it is extremely hard to decipher for non-maintainers) |
That is a really good question, the design did change a lot since the first prototyping, and a high level documentation would be probably good too. |
Here's a tracking issue for the documentaiton: |
There isn't really anything yet. I'll try to put something together next week. If you're interested in something to read now, the packet format is very close to what is described in the Sphinx paper. |
From what I can see, after the initial sync the node will wait for the registration phase of the next session before registering. How does it it handle traffic in the meantime and what key will be used? Would it make sense to register immediatelly after the sync is completed? |
If a node isn't registered as a mixnode it should only receive reply and loop cover traffic, which it will generate a random key pair for. The public key doesn't need to be shared with any other nodes. If a node is registered as a mixnode and then restarts, it will no longer have the private key corresponding to the public key that was registered, so it will only be able to operate as a non-mixnode until the next session. I haven't really thought about allowing registrations after the start of a session. Maybe this could be made to work, but it seems like a bunch of extra complexity for little gain. Sessions are only a few hours long, and if a node is an active validator in a session, it should really stay online for the whole session. |
User @zdave-parity, please sign the CLA here. |
Done this now. And got rid of the |
Been on holiday for 2 weeks, just getting back to this. Re moving the registration stuff from an offchain worker to a separate call into the runtime: I can do this, but it will involve duplicating some of the stuff in It's not clear to me why the offchain stuff is handled differently to the other execution extensions -- why is the |
Responsible for determining the mixnode set for each session.
I've reworked things to get rid of the offchain worker. The mixnet service now just calls the |
@bkchr Could you please take another look? |
@@ -0,0 +1,67 @@ | |||
// This file is part of Substrate. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would also be really nice to move this into sc-mixnet
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm I'm actually thinking the kitchen-sink node would be the right place for this? It makes assumptions that are not made by sc-mixnet
. For example, sc-mixnet
does not assume that non-authorities cannot be mixnodes (sc-mixnet
doesn't care whether nodes are authorities or not). Similarly, sc-mixnet
doesn't really know anything about RPC (so the comments about RPC would be out of place).
If you agree, I'll move it there instead.
Not necessary; the mixnet pallet can track the session index itself.
To avoid node depending on pallet_mixnet.
In particular, avoid referring to "the register phase". Just refer to it as the DisconnectFromPrev phase, which is the name of the enum variant.
Note that registrations are now accepted in all phases, and the previous mixnode set is kept for the whole session.
The CI pipeline was cancelled due to failure one of the required jobs. |
See #13904.
This adds all the necessary mixnet components, and puts them together in the "kitchen-sink" node/runtime. The components added are:
frame/mixnet
). This is responsible for determining the current mixnet session and phase, and the mixnodes to use in each session. It includes an offchain worker which attempts to automatically register validator nodes as mixnodes. The logic of this pallet is very similar to that of theim-online
pallet.client/mixnet
). This implements the core mixnet logic, building on the mixnet crate. The service communicates with other nodes using notifications sent over the "mixnet" protocol.