Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mixnet integration #1346

Merged
merged 8 commits into from
Oct 9, 2023
Merged

Mixnet integration #1346

merged 8 commits into from
Oct 9, 2023

Conversation

zdave-parity
Copy link
Contributor

See #1345, paritytech/substrate#14207.

This adds all the necessary mixnet components, and puts them together in the "kitchen-sink" node/runtime. The components added are:

  • A pallet (frame/mixnet). This is responsible for determining the current mixnet session and phase, and the mixnodes to use in each session. It provides a function that validators can call to register a mixnode for the next session. The logic of this pallet is very similar to that of the im-online pallet.
  • A service (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.
  • An RPC interface. This currently only supports sending transactions over the mixnet.

@zdave-parity zdave-parity requested review from arkpar and bkchr and removed request for arkpar August 31, 2023 23:34
@paritytech-ci paritytech-ci requested a review from a team August 31, 2023 23:35
@zdave-parity
Copy link
Contributor Author

Moved PR from the old Substrate repo (old PR here). I think I've addressed all the comments except two...

Not sure where the CLI param stuff should go. On the Substrate PR I made this comment:

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).

Not sure how log levels should be picked. On the Substrate PR I made this comment:

Is there a reference somewhere for which log levels should be used when? For this mixnet stuff I have so far been using error for things which should only happen if some node misbehaves, warn for things which might happen even if all nodes behave (but isn't likely to), and debug for everything else.

@arkpar
Copy link
Member

arkpar commented Sep 1, 2023

Not sure how log levels should be picked.

info for informational messages that are useful to the user. Progress, detected network addresses, etc.

error should be used for fatal errors that typically lead to process termination.

warn is for

  1. Things that the user can take action for. E.g. misconfiguration.
  2. Invalid/suspicious state caused by logic bugs.

debug and trace should be used for everything else with debug being higer priority/lower verbosity.

So normally, I'd say network misbehaviour should be a debug, since there's nothing that can be done by the user about it. Unless we can detect that something is terribly wrong and show a general warning. E.g. when majority is misbehaving.

@arkpar
Copy link
Member

arkpar commented Sep 1, 2023

Not sure where the CLI param stuff should go. On the Substrate PR I made this comment:

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).

I'm fine with keeping it in sc-cli

@zdave-parity
Copy link
Contributor Author

Thanks. Fixed up all the log levels.

Comment on lines +47 to +49
///
/// `session_index` should match `session_status().current_index`; if it does not, `false`
/// is returned immediately.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't get this requirement. If you know the current_index why do I need to pass it then?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's just to avoid needing two calls into the runtime every block (one to check the session index and one to register). I can drop this argument if you'd prefer the two calls.

substrate/frame/mixnet/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/mixnet/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/mixnet/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/mixnet/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/mixnet/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/mixnet/src/lib.rs Outdated Show resolved Hide resolved
Comment on lines +477 to +487
///
/// `session_index` should match `session_status().current_index`; if it does not, `false` is
/// returned immediately.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment as for the runtime api. I don't get why we not just fetch the session index?

substrate/frame/mixnet/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/mixnet/src/lib.rs Outdated Show resolved Hide resolved
@zdave-parity
Copy link
Contributor Author

Does anyone have any idea what's going on with the Zombienet block-building test (https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3884822)? The CI output is not particularly enlightening.

@pepoviola
Copy link
Contributor

Does anyone have any idea what's going on with the Zombienet block-building test (https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/3884822)? The CI output is not particularly enlightening.

We should update the version of zombienet used in ci to the latest v1.3.69 that includes your pr in zombienet code 👍 .

@zdave-parity
Copy link
Contributor Author

This PR doesn't actually depend on that change as it doesn't include the Zombienet test; I was going to add that in a follow-up commit.

It looks like there is some issue starting the nodes up in the block building test but I'm not sure how to start debugging this. The test works fine locally using the native backend, but this isn't exactly what CI is running. If I could run what is being run under CI locally that would be a good start but I'm not sure how to do that.

@pepoviola
Copy link
Contributor

This is the error, the node crash.

2023-10-05 14:19:35.897	
Error: Service(Other("Error parsing spec file: missing field `mixnet` at line 129 column 13"))

https://grafana.teleport.parity.io/goto/PZf_nnGIg?orgId=1

@zdave-parity
Copy link
Contributor Author

Ah ok, I guess that change is required then, my mistake. Thanks!

impl<T: Config> Pallet<T> {
/// Register a mixnode for the following session.
#[pallet::call_index(0)]
#[pallet::weight(1)] // TODO
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this need a benchmark?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but this pallet doesn't go to production directly?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, can be done as a follow-up, but our policy is to use tracking issues instead of TODOs :P

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, fair point.

@zdave-parity zdave-parity merged commit a808a3a into paritytech:master Oct 9, 2023
8 checks passed
ordian added a commit that referenced this pull request Oct 12, 2023
* master: (33 commits)
  ci: set CI_IMAGE back to (now updated) .ci-unified (#1854)
  ci: bump ci image to rust 1.73.0 (#1830)
  Refactor Identity to benchmark v2 (#1838)
  PVF worker: bump landlock, update ABI docs (#1850)
  Xcm emulator nits (#1649)
  Fixes path issue in derive-impl (#1823)
  upgrade to macro_magic 0.4.3 (#1832)
  Use safe math when pruning statuses (#1835)
  remote-ext: fix state download stall on slow connections and reduce memory usage (#1295)
  Update testnet bootnode dns name (#1712)
  [FRAME] Warn on unchecked weight witness (#1818)
  [xcm] Use `Weight::MAX` for `reserve_asset_deposited`, `receive_teleported_asset` benchmarks (#1726)
  Update bridges subtree (#1803)
  Check for parent of first ready block being on chain (#1812)
  Make CheckNonce refuse transactions signed by accounts with no providers (#1578)
  Fix Asset Hub collator crashing when starting from genesis (#1788)
  Mixnet integration (#1346)
  [xcm-emulator] Decouple the `AccountId` type from `AccountId32` (#1458)
  Treasury spends various asset kinds (#1333)
  chore: bump zombienter version (#1806)
  ...
@zdave-parity zdave-parity deleted the mixnet branch December 22, 2023 13:48
bgallois pushed a commit to duniter/duniter-polkadot-sdk that referenced this pull request Mar 25, 2024
See paritytech#1345, <paritytech/substrate#14207>.

This adds all the necessary mixnet components, and puts them together in
the "kitchen-sink" node/runtime. The components added are:

- A pallet (`frame/mixnet`). This is responsible for determining the
current mixnet session and phase, and the mixnodes to use in each
session. It provides a function that validators can call to register a
mixnode for the next session. The logic of this pallet is very similar
to that of the `im-online` pallet.
- A service (`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.
- An RPC interface. This currently only supports sending transactions
over the mixnet.

---------

Co-authored-by: David Emett <[email protected]>
Co-authored-by: Javier Viola <[email protected]>
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 8, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 9, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
serban300 pushed a commit to serban300/polkadot-sdk that referenced this pull request Apr 10, 2024
bkchr pushed a commit that referenced this pull request Apr 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T0-node This PR/Issue is related to the topic “node”.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants