Switch GrandPa to std futures (replaces #3909) #4612
Conversation
|
@rphmeier The changes needed for https://github.com/paritytech/finality-grandpa can be found here: https://github.com/expenses/finality-grandpa/tree/future |
|
@expenses Do you have a PR ready to go? Differences from paritytech/finality-grandpa#100 ? |
expenses
added
the
A3-in_progress
tomaka
left a comment
tomaka
left a comment
There was a problem hiding this comment.
LGTM after the nit with map
|
We should probably field test this PR pretty heavily. |
|
@andresilva @rphmeier suggested I ping you for ideas on how to test this. |
|
@expenses https://gist.github.com/andresilva/264e00476f125d3bd6716fb58478d543 have a look at these instructions for setting up a local testnet, feel free to dm me on riot if you have any issues. Ideally, we should also build and deploy polkadot with these changes on one of our validators to do some field testing, coordinating with @ddorgan (we might have to wait a bit for this due to the issues with latest libp2p version). |
|
@andresilva I don't have the hardware to run the dockerised local testnet right now, but perhaps we could fieldtest with @ddorgan? |
|
I guess we could deploy some validators on flaming fir (or spin up some temporary testnet) with these changes? cc @ddorgan |
expenses
removed
the
A3-in_progress
mxinden
left a comment
mxinden
left a comment
There was a problem hiding this comment.
Thanks for all the effort here.
I have a couple of questions and comments. None of them are important enough to block this pull request.
| target_hash: block_30_hash, | ||
| }; | ||
|
|
||
| Pin::new(&mut *round_tx.lock()).start_send(finality_grandpa::Message::Prevote(prevote)).unwrap(); |
There was a problem hiding this comment.
One should first call poll_ready before start_send on Sink. As a convenience function, given that we are within an async block, one can call SinkExt::send.
(I don't think this was introduced through this pull request, but the pull request enables the easy fix via SinkExt::send. Let me know if you want me to do this as a follow up.)
There was a problem hiding this comment.
We actually can't use SinkExt::send, because that's that'd mean awaiting a function that borrows a mutex and ends up with: *mut () cannot be sent between threads safely.
Seems to be finalizing 1 day later! |
|
Flaming fir was having some issues when running nodes without this PR anyway, so I think it's probably unrelated (it's still running the libp2p version that we had issues with). I'm OK with not blocking this PR further, when we start using this in polkadot we can do some prior testing on our nodes as well. I'll give this a final review in a bit. |
Co-Authored-By: André Silva <[email protected]>
|
Ok. If we're all happy with merging this PR, I'll do that in a few hours. |
This is an updated branch of #3909.