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

Add sync backing logic & refactor client to be closer to AURA api #14

Merged
merged 34 commits into from
Jan 4, 2024

Conversation

librelois
Copy link
Collaborator

@librelois librelois commented Dec 5, 2023

This PR include two distincts changes:

  • A refactoring of the client part of nimbus to have an API closer to AURA, the aim is to facilitate future maintenance and migration to async backing.
  • Addition of a pallet dedicated to async backing. This new pallet implements the ConsensusHook trait and stores information about the last slot (last slot number and number of blocks produced in it).
    • The new pallet introduce a trait GetAndVerifySlot to keep the pallet agnostic of the slot implementation. If your parachain don't need slots at the parachain level, you can use the RelaySlot implementation. Otherwise, use the ParaSlot implementation.

Implement the slot logic

Without parachain slot

impl async_backing::Config for Runtime {
    ...
    type GetAndVerifySlot = RelaySlot;
}

With parachain slot

struct SlotProvider;
impl Get<(Slot, SlotDuration)> for SlotProvider {
    fn get() -> (Slot, SlotDuration) {
        // Implement your logic here to get the current slot and the slot duration
    }
}

impl async_backing::Config for Runtime {
    ...
    type GetAndVerifySlot = ParaSlot<RELAY_CHAIN_SLOT_DURATION_MILLIS, SlotProvider>;
}

pallets/nimbus-async-backing/src/consensus_hook.rs Outdated Show resolved Hide resolved
pallets/nimbus-async-backing/src/lib.rs Outdated Show resolved Hide resolved
pallets/nimbus-async-backing/src/lib.rs Outdated Show resolved Hide resolved
@librelois librelois changed the title Add sync backing logic (runtime only) & refactor client to be closer to AURA api Add sync backing logic & refactor client to be closer to AURA api Dec 12, 2023
@librelois librelois marked this pull request as ready for review December 12, 2023 18:07
Copy link
Contributor

@Agusrodri Agusrodri left a comment

Choose a reason for hiding this comment

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

The changes look great so far, awesome work @librelois! Once we have everything compiling, we should start testing how the node behaves in terms of collation before merging. I compared your changes against the polkadot-parachain ones and everything seems to be correct (and also the nimbus adaptation).

@librelois
Copy link
Collaborator Author

librelois commented Dec 13, 2023

@Agusrodri it compiles now

@Agusrodri
Copy link
Contributor

Agusrodri commented Dec 14, 2023

While trying to set these changes on Tanssi, I wasn't being able to compile the project. I was receiving the following error while doing cargo check --release:

error[E0433]: failed to resolve: use of undeclared crate or module `std`
  --> primitives/async-backing/src/lib.rs:26:1
   |
26 | / sp_api::decl_runtime_apis! {
27 | |     /// This runtime API is used to inform potential block authors whether they will
28 | |     /// have the right to author at a slot, assuming they have claimed the slot.
29 | |     ///
...  |
49 | |     }
50 | | }
   | |_^ use of undeclared crate or module `std`
   |
   = note: this error originates in the macro `sp_api::decl_runtime_apis` (in Nightly builds, run with -Z macro-backtrace for more info)

And also this one:

error[E0433]: failed to resolve: use of undeclared crate or module `std`
   --> primitives/nimbus-primitives/src/lib.rs:168:1
    |
168 | / sp_api::decl_runtime_apis! {
169 | |     /// The runtime api used to predict whether a Nimbus author will be eligible in the given slot
170 | |     pub trait NimbusApi {
171 | |         fn can_author(author: NimbusId, relay_parent: u32, parent_header: &Block::Header) -> bool;
172 | |     }
173 | | }
    | |_^ use of undeclared crate or module `std`
    |
    = note: this error originates in the macro `sp_api::decl_runtime_apis` (in Nightly builds, run with -Z macro-backtrace for more info)

error[E0412]: cannot find type `String` in this scope
   --> primitives/nimbus-primitives/src/lib.rs:154:2
    |
154 |     app_crypto!(sr25519, crate::NIMBUS_KEY_ID);
    |     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ not found in this scope
    |
    = note: this error originates in the macro `$crate::app_crypto_pair_functions_if_std` which comes from the expansion of the macro `app_crypto` (in Nightly builds, run with -Z macro-backtrace for more info)

The problems got solved by removing the default-features = false from async-backing-primitives and nimbus-primitives packages. I guess this makes sense as both of them strongly depend on std features on the client side, and I think disabling std by default was bringing issues due to that.

Given this, what I suggest is to enable std by default for these two packages, and disable std for them only inside the runtime (if needed), what do you think?

Note: the client package located in this branch also doesn't compile and throws the same errors, so I think it will be an issue if we don't handle it. Let me know your opinion :)

@librelois
Copy link
Collaborator Author

Given this, what I suggest is to enable std by default for these two packages, and disable std for them only inside the runtime (if needed), what do you think?

You should do exactly the opposite, std features should be disabled by default and explicitly enabled on the client-side, it's the way substrate work.

Note: the client located in this branch also doesn't compile and throws the same errors, so I think it will be an issue if we don't handle it. Let me know your opinion :)

The moonkit template client compile well in this CI and locally with command cargo build --release -p moonkit-template, so what do you mean?

@Agusrodri
Copy link
Contributor

Agusrodri commented Dec 14, 2023

You should do exactly the opposite, std features should be disabled by default and explicitly enabled on the client-side, it's the way substrate work.

Yes I know that's the normal way of configuring it, but seems that's not the case for these two deps, I don't know to be honest.

Edit: Adding the std feature only to the client package works fine after running cargo clean.

The moonkit template client compile well in this CI and locally with command cargo build --release -p moonkit-template, so what do you mean?

What I mean is that the client package is not compiling separately (actually is the nimbus-consensus one), to reproduce the errors you can do cargo check --release inside client/consensus/nimbus-consensus.

@librelois librelois merged commit a7fc337 into main Jan 4, 2024
8 of 10 checks passed
@librelois librelois deleted the elois-async-backing branch January 4, 2024 10:21
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

Successfully merging this pull request may close these issues.

2 participants