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

Reuse subsystems in different orchestra #32

Open
skunert opened this issue Feb 14, 2023 · 5 comments
Open

Reuse subsystems in different orchestra #32

skunert opened this issue Feb 14, 2023 · 5 comments

Comments

@skunert
Copy link
Contributor

skunert commented Feb 14, 2023

Some time ago, we introduced a minimal Polkadot node in Cumulus. It runs a subset of subsystems that is usually present in the polkadot node. For the initial implementation, I replaced unneeded subsystems with DummySubsystem.

This works quite well, but I would like to clean this up and define a new orchestra that only accepts the required subsystem set.

Naive definition of a new orchestra and usage with polkadot subsystems gives this error:

error[E0271]: type mismatch resolving `<CollatorOverseerSubsystemContext<AvailabilityRecoveryMessage> as SubsystemContext>::OutgoingMessages == AvailabilityRecoveryOutgoingMessages`
   --> client/relay-chain-minimal-node/src/collator_overseer.rs:167:4
    |
167 |         .availability_recovery(AvailabilityRecoverySubsystem::with_chunks_only(
    |          ^^^^^^^^^^^^^^^^^^^^^ expected enum `polkadot_overseer::AvailabilityRecoveryOutgoingMessages`, found enum `collator_overseer::AvailabilityRecoveryOutgoingMessages`
    |
    = note: enum `collator_overseer::AvailabilityRecoveryOutgoingMessages` and enum `polkadot_overseer::AvailabilityRecoveryOutgoingMessages` have similar names, but are actually distinct types

I guess this is expected since we define the trait location via prefix manually. So it seems like there is a 1:1 mapping between subsystem <-> orchestra?

Before diving deeper into this, I would like to have an opinion from someone more experienced with the orchestra (maybe @drahnr?). Is this even supported currently? How would I go about this?

@drahnr
Copy link
Collaborator

drahnr commented Feb 14, 2023

The issue with declaring tow #[orchestra::orchestra(..)] annotated types, with similar subsystems, yields two types of identical name, and a bunch of internal types with identical names than the original ones, but they don't have the same type id from a rustc perspective/are disjunct types. Hence why you run into the issue.

I only have ugly solutions for you. Happy to share if interested. (Hint: Modified import paths in all subsystems based on a feature-flag, I strongly recommend against that).

@drahnr
Copy link
Collaborator

drahnr commented Feb 14, 2023

The best approach might be to make the macro #[cfg(feature =.. )] aware and allow conditional compilation of fields of one orchestra declarated struct Foo.

@skunert
Copy link
Contributor Author

skunert commented Feb 15, 2023

The best approach might be to make the macro #[cfg(feature =.. )] aware and allow conditional compilation of fields of one orchestra declarated struct Foo.

So basically we would pass different feature flags depending on whether we compile for cumulus or polkadot and then disable some of the subsystems, but reuse the same orchestra? Did I understand that correctly?

One other solution (also hacky) I was thinking about was to introduce one more abstraction level below the macro level. And then reuse the same code but declare the subsystems separately in cumulus.

@drahnr
Copy link
Collaborator

drahnr commented Feb 15, 2023

The best approach might be to make the macro #[cfg(feature =.. )] aware and allow conditional compilation of fields of one orchestra declarated struct Foo.

So basically we would pass different feature flags depending on whether we compile for cumulus or polkadot and then disable some of the subsystems, but reuse the same orchestra? Did I understand that correctly?

Yes, that's correct.

One other solution (also hacky) I was thinking about was to introduce one more abstraction level below the macro level. And then reuse the same code but declare the subsystems separately in cumulus.

I am not sure I understand what you mean. Also note, that this is quite a bit of work and would add further complexity to the proc-macro, which is not desirable.

So far, the DummySubsystems seem to be the easiest path.

Chaninging the number of subsystems will percolate through quite some code in polkadot (i.e. the overseer down to service and cli due to the impl OverseerGen which takes args for all subsystems, it's specific to polkadot, still something to consider).

Let me think about this for a few days, I have a hunch where we could go with something like !/Infallible as a subsystem of which the orchestra proc-macro is aware.

Let's do a call on Friday~ish?

@skunert
Copy link
Contributor Author

skunert commented Feb 15, 2023

Thanks for the help! I think a call might be useful!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants