Skip to content
This repository has been archived by the owner on Feb 3, 2022. It is now read-only.

Update master branch to v2.0.0 #81

Closed
wants to merge 2 commits into from
Closed

Update master branch to v2.0.0 #81

wants to merge 2 commits into from

Conversation

danforbes
Copy link

Relates to #79 and #80. I am also working on updating the workshop branch to v2.0.0

CC: @JoshOrndorff

@danforbes danforbes marked this pull request as draft January 2, 2021 18:01
@danforbes
Copy link
Author

Marking the 2.0 versions as drafts because it seems that when they are run in development mode they do not produce blocks 🤔

Copy link
Owner

@JoshOrndorff JoshOrndorff left a comment

Choose a reason for hiding this comment

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

Looks good after just looking it over. The removal of the mine function was a surprise, but I think I remember that changing in the recipes also right?

@@ -48,7 +50,7 @@ The following example takes you through a scenario where:

1. Compile and build a release node
```bash
cargo build --release
cargo +nightly-2020-10-05 build --release
Copy link
Owner

Choose a reason for hiding this comment

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

@@ -197,7 +195,7 @@ pub fn new_light(config: Configuration, sr25519_public_key: sr25519::Public) ->
client.clone(),
Sha3Algorithm::new(client.clone()),
0, // check inherents starting at block 0
Some(select_chain),
select_chain,
inherent_data_providers.clone(),
// TODO what should I Really use here? This compiles, and the `can_author_with` from line 196 doesn't??
Copy link
Owner

Choose a reason for hiding this comment

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

Is "line 196" still a valid reference?

@@ -197,7 +195,7 @@ pub fn new_light(config: Configuration, sr25519_public_key: sr25519::Public) ->
client.clone(),
Sha3Algorithm::new(client.clone()),
0, // check inherents starting at block 0
Some(select_chain),
select_chain,
inherent_data_providers.clone(),
// TODO what should I Really use here? This compiles, and the `can_author_with` from line 196 doesn't??
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// TODO what should I Really use here? This compiles, and the `can_author_with` from line 196 doesn't??
// TODO what should I Really use here? This compiles, and the `can_author_with` from above doesn't??

@apopiak
Copy link
Contributor

apopiak commented Jan 5, 2021

The removal of the mine function was a surprise, but I think I remember that changing in the recipes also right?

I have this question, too, LGTM otherwise

@danforbes
Copy link
Author

@JoshOrndorff & @apopiak - mine is not in the trait in v2.0.0 🤷🏻 https://substrate.dev/rustdocs/v2.0.0/sc_consensus_pow/trait.PowAlgorithm.html

Regarding the fact that blocks are not being authored in --dev mode, Joshy suggested the following:

What if you add --validator? If that solves it then there is some easy code we could add to service.rs to fix --dev mode.

@apopiak
Copy link
Contributor

apopiak commented Jan 6, 2021

When I add --validator or --alice it still does not seem to produce blocks.

@apopiak
Copy link
Contributor

apopiak commented Jan 6, 2021

The miner is started but does not seem to do anything useful?

@@ -153,19 +153,17 @@ pub fn new_full(config: Configuration, sr25519_public_key: sr25519::Public) -> R
let can_author_with =
sp_consensus::CanAuthorWithNativeVersion::new(client.executor().clone());

sc_consensus_pow::start_mine(
sc_consensus_pow::start_mining_worker(
Copy link
Contributor

Choose a reason for hiding this comment

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

This is started, but does not seem to actually mine anything.

@jimmychu0807
Copy link

Closing it as it has been a while.

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

Successfully merging this pull request may close these issues.

4 participants