Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@tomaka
Copy link
Contributor

@tomaka tomaka commented Oct 24, 2019

Transitions the code of substrate-finality-grandpa to Futures from the Rust standard library.

cc #3099

A few notes:

  • All the changes are straight-forward. No logic has been touched. I know that the GrandPa code is sensible and have been extra-cautious.
  • While stable futures, contrary to old futures, don't necessarily need to produce errors, for now I only removed error possibilities when it was obvious (e.g. internal channels that can no longer produce errors).
  • I converted telemetry_connection_sinks in substrate-service to futures 0.3, but couldn't do the same for on_exit because there's this weird against-the-design-of-futures cloning capability. This will need to be done separately later after some brainstorming.
  • I had to hard-code the H256 type for block hashes in communication/mod.rs, otherwise the compiler wouldn't be able to infer it. This is probably related to the fact that the items of the new Sink trait are now a regular generic parameter, rather than an associated type. The H256 block hash type is already hardcoded in GrandPa, so this is not a huge deal.
  • This is unfortunately an API breaking change for nodes, because they have to add some compatibility shim when creating the Service.

Pre-requirement before merging: publish a new version of the finality-grandpa crate, and update the Cargo.toml of this PR.

@tomaka tomaka added A0-please_review Pull request needs code review. B2-breaksapi labels Oct 24, 2019
@tomaka tomaka requested review from andresilva and rphmeier October 24, 2019 15:59
@tomaka tomaka requested a review from Demi-Marie as a code owner October 24, 2019 15:59
@tomaka
Copy link
Contributor Author

tomaka commented Oct 25, 2019

I naively forgot to fix the tests, but fixing the tests is quite complicated because the network (used in tests) still uses futures 0.1.

Still needs a bit of work.

@tomaka tomaka added A1-onice and removed A0-please_review Pull request needs code review. labels Oct 25, 2019
@kigawas
Copy link
Contributor

kigawas commented Oct 28, 2019

I naively forgot to fix the tests, but fixing the tests is quite complicated because the network (used in tests) still uses futures 0.1.

Still needs a bit of work.

I suggest migrating network to new futures first, but that part would also be kind of painful

@tomaka
Copy link
Contributor Author

tomaka commented Oct 28, 2019

I suggest migrating network to new futures first, but that part would also be kind of painful

We can't actually migrate the network, as it's waiting for libp2p. We could, however, use the compatibility layer to make it look like the network has migrated.

However there's a chicken-and-egg problem. If we upgrade the network first, then the GrandPa tests will also fail to compile, as GrandPa would still use old futures.
We might have to upgrade GrandPa and the network together.

@kigawas
Copy link
Contributor

kigawas commented Oct 28, 2019

However there's a chicken-and-egg problem. If we upgrade the network first, then the GrandPa tests will also fail to compile, as GrandPa would still use old futures.

How about upgrading the network to 0.3 futures and compat it as 0.1?

@tomaka
Copy link
Contributor Author

tomaka commented Oct 28, 2019

This code is surprisingly tricky to upgrade: https://github.com/paritytech/substrate/blob/master/core/finality-grandpa/src/communication/mod.rs#L352-L361

One way to do this could be to use async_std::task::spawn in order to spawn the background futures. However the async_std library doesn't compile on stable right now, as it assumes async/await.

We might wait until 7th of November (stabilization of async/await) to finish this PR.

Copy link
Contributor

@Demi-Marie Demi-Marie left a comment

Choose a reason for hiding this comment

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

LGTM but I will wait on someone else to make the final call here.

.compat()
);
let inner_rx: Pin<Box<dyn Stream<Item = _> + Send>> =
Box::pin(gossip.messages_for(GRANDPA_ENGINE_ID, topic).map(Result::Ok));
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
Box::pin(gossip.messages_for(GRANDPA_ENGINE_ID, topic).map(Result::Ok));
Box::pin(gossip.messages_for(GRANDPA_ENGINE_ID, topic).map(Ok));

futures = "0.1.29"
futures03 = { package = "futures-preview", version = "0.3.0-alpha.19", features = ["compat"] }
futures01 = { package = "futures", version = "0.1.29" }
futures-preview = { version = "0.3.0-alpha.19", features = ["compat"] }
Copy link
Contributor

Choose a reason for hiding this comment

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

Futures 0.3 has been released

TestBlockSyncRequester::default(),
block_status,
global_rx.map_err(|_| panic!("should never error")),
global_rx.map_err(|()| panic!("should never error")),
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs a better justification. If there is no possible error, we should use a void type here. It is possible to implement a void type without an ICE.

@tomaka tomaka closed this Jan 13, 2020
@tomaka tomaka deleted the grandpa-new-futures branch January 13, 2020 15:29
expenses added a commit that referenced this pull request Jan 24, 2020
* Switch GrandPa to new futures

* Work on making tests work

* until_imported tests working again

* Work on switching tests to stable futures

* Modifications

* Re-add test as #[ignore]

* Don't ignore

* Add manual unpins

* Remove Header import

* Return concrete Sink type

* Switch to crates.io finality-grandpa version

* Remove use statement that slipped in

* Fix some nitpicks

* Remove unpin from i

* Fixed typo

* Move futures01 to dev-deps

* Fix nitpicks

* Update client/finality-grandpa/src/communication/mod.rs

Co-Authored-By: André Silva <[email protected]>

* nitpicking

Co-authored-by: Pierre Krieger <[email protected]>
Co-authored-by: André Silva <[email protected]>
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