core/finality-grandpa: Request block sync from network after import timeout #3800

mxinden · 2019-10-11T12:42:59Z

Goal

In order to participate in the Grandpa gossip process a Grandpa voter needs
to have access to the blocks referenced in the Grandpa messages. Blocks are
being synchronized atonomously via the network sync service. In case this does
not work as expected e.g. due to delayed messages where the corresponding blocks
are already purged, we can use the new set_sync_fork_request API added in
#3633 to tell the network:

Which blocks are relevant for the current round.
Where to retrieve these relevant blocks from.

This is further described in #3629.

An initial solution piggy-backing on the Grandpa neighbor messages was
suggested in #3755. After further discussions an easier (intermediate) solution
via Grandpa vote messages and the UntilImported component is suggested with
this pull request. In addition the UntilImported component already has the
notion of withholding messages and the notion of a import timeout.

2nd solution (Grandpa vote messages and UntilImported)

The UntilImported component within core/finality-grandpa withholds messages
from the upper Grandpa layers until all blocks referenced in these messages
are imported on the local node.

With the goal above in mind we can notify the network sync service after a
certain timeout of waiting that we need a given block and which peers probably
have it.

Pull request status

I am opening this up for architectual feedbacks. Changes are grouped by their
functionality. This pull request replaces #3755. I will follow up on comments in
#3755 in later pull requests.

…rted

rphmeier · 2019-10-12T00:27:39Z

core/finality-grandpa/src/communication/mod.rs


-		let incoming = incoming.select(out_rx);
+		// TODO: Do I understand correctly that we combine the incoming Grandpa messages with the messages we are
+		// sending out to other nodes and in addition thereby also send to ourself?


yes! this means that the logic for importing our own votes is unified with the logic for importing others' votes in the GRANDPA crate - reducing code paths in the vote processing pipeline and making bugs less likely.

mxinden · 2019-10-16T13:08:44Z

core/finality-grandpa/src/until_imported.rs

 					let next_log = *last_log + LOG_PENDING_INTERVAL;
+					// TODO: Should this not be >=? We want to log whenever we want to log every LOG_PENDING_INTERVAL,
+					// right?
 					if Instant::now() <= next_log {


@rphmeier this should be >=, right?

The UntilImported component should log whenever waiting for a specific block to be imported surpassed a defined constant timeout. Without this patch the code would log whenever the current time was below the timeout.

andresilva

Needs some cleaning up but I will post a more thorough review later. I have tested this PR under the chaos net, it seems to be working correctly, nodes always managed to get the block data they needed to import votes (and I checked that requests were indeed going through explicit fork sync sometimes).

The only problem with the current implementation is that we will only ever request to sync from the peer that originally sent us the vote, even as we see the same vote being broadcast to us from other peers. The reason is that the low-level gossip layer keeps track of known messages, so subsequent receivals of the same message won't reach the higher layers (i.e. until_imported won't ever see a duplicate message). I am not sure how to fix this easily while keeping this logic in until_imported.

@rphmeier Any thoughts on this?

andresilva · 2019-10-17T11:44:15Z

core/finality-grandpa/src/until_imported.rs

+	/// Mapping block hashes to their block number, the point in time it was first encountered (Instant), nodes that
+	/// have the corresponding block (inferred by the fact that they send a message referencing it) and a list of
+	/// Grandpa messages referencing the block hash.
+	pending: HashMap<Block::Hash, (NumberFor<Block>, Instant, Vec<network::PeerId>, Vec<(M)>)>,


This should be a HashSet<PeerId>, we might see multiple messages from the same peer referencing the same hash.

andresilva · 2019-10-17T15:49:54Z

Regarding the issue I mentioned above in my review. We could start by asking the peer that sent us the vote, and then fallback to asking any peer we're connected to that is on the same round or further from the round whose block data we're looking for.

Another issue with the current implementation is that only validators will be actively trying to fetch block data. Full nodes just running the GRANDPA observer don't create any round streams and therefore don't run the code in UntilImported. An easy fix is to have the observer create the round streams and just consume and drop all messages, this will make sure that the logic in UntilImported is run.

…_until_imported

…locks" This reverts commit 61ac9dd.

…orted" This reverts commit afdc964.

When requesting an explicit fork sync, try to sync from all known peers, when no specific peers were provided.

arkpar · 2019-10-23T12:29:41Z

core/network/src/protocol/sync.rs

+				hash, peers,
+			);
+
+			peers = self.peers.iter().map(|(id, _)| id.clone()).collect();


Should filter out peers that have best_block < number. We don't want to request anything from peers that are definitely not yet synced.

When making an explicit fork sync request without specifying any peers, make sure to only request it from the locally known peers that are either ahead or on a par compared to the block number we are looking for.

…_until_imported

andresilva · 2019-10-23T16:17:22Z

core/finality-grandpa/src/environment.rs

 /// The environment we run GRANDPA in.
 pub(crate) struct Environment<B, E, Block: BlockT, N: Network<Block>, RA, SC, VR> {
+	// TODO: As far as I can tell this does not follow the pattern of having an outer struct protecting an inner struct
+	// via a lock in order to be Sync. How about renaming this to `client` like we do in many other places?


Indeed, I did the rename.

andresilva · 2019-10-23T16:41:42Z

core/finality-grandpa/src/until_imported.rs

-	status_check: Status,
+	block_sync_requester: BlockSyncRequester,
+	status_check: BlockStatus,
+	// TODO: Why is this called inner? Why not being more descriptive and say finality_msg_stream?


This is used with both the global per-set streams (commits, catch-ups) and the round streams (votes). Maybe rename it to msg_stream?

I removed the TODO but we can address this in a follow up PR if this one gets merged soon.

andresilva · 2019-10-23T16:43:30Z

core/finality-grandpa/src/until_imported.rs

+			for (&block_hash, &mut (block_number, ref mut last_log, ref v)) in &mut self.pending {
 				if let Some(number) = self.status_check.block_number(block_hash)? {
+					// TODO: Now that we pass the number all the way down here, should we check if it is the same as we
+					// are expecting it to be?


This will be checked later on in wait_completed, we pass the canon number to it (the one we fetch here and store in known_keys. The wait_completed implementations can look at the vote itself and the target number to figure out what the original number was.

andresilva

lgtm

mxinden

Additional commits from @andresilva look good to me. @arkpar do you have further thoughts on this?

mxinden added 9 commits October 10, 2019 16:28

core/finality-grandpa: Pass Grandpa msg sender up to UntilImported

afdc964

core/finality-grandpa: Track senders to maybe later request blocks

61ac9dd

core/finality-grandpa: Make BlockStatus pub only within crate

ce9db3f

core/finality-grandpa: Abstract NetworkBridge with BlockSyncRequester

3cd6de0

core/finality-grandpa: Pass BlockSyncRequester to UntilImported

fd4800d

core/finality-grandpa: Track block number of pending within UntilImpo…

0f82795

…rted

core/finality-grandpa: Request block sync on long wait

a2f6684

core/finality-grandpa: Adjust unit tests to previous changes

9f9e15f

core/finality-grandpa: Fix line length

126c35e

mxinden added the M4-core label Oct 11, 2019

mxinden requested review from andresilva, arkpar and rphmeier October 11, 2019 12:42

mxinden changed the title ~~Sync request in until imported~~ core/finality-grandpa: Request block sync from network after import timeout Oct 11, 2019

mxinden mentioned this pull request Oct 11, 2019

core/finality-grandpa: Gossip synced blocks via neighbor packets #3755

Closed

rphmeier reviewed Oct 12, 2019

View reviewed changes

core/finality-grandpa: Add comment explaining in & out vote combination

72516c0

mxinden commented Oct 16, 2019

View reviewed changes

mxinden marked this pull request as ready for review October 16, 2019 19:37

mxinden requested a review from Demi-Marie as a code owner October 16, 2019 19:37

core/finality-grandpa: Log after, not before, timeout expired

3b4adca

The UntilImported component should log whenever waiting for a specific block to be imported surpassed a defined constant timeout. Without this patch the code would log whenever the current time was below the timeout.

mxinden added the A0-please_review Pull request needs code review. label Oct 17, 2019

andresilva reviewed Oct 17, 2019

View reviewed changes

mxinden added 5 commits October 18, 2019 10:12

core/finality-grandpa: Collect senders as HashSet for deduplication

74d6637

Merge remote-tracking branch 'paritytech/master' into sync_request_in…

c138cfb

…_until_imported

Revert "core/finality-grandpa: Track senders to maybe later request b…

6a1eec8

…locks" This reverts commit 61ac9dd.

Revert "core/finality-grandpa: Pass Grandpa msg sender up to UntilImp…

8e7663c

…orted" This reverts commit afdc964.

core/network/sync: Ask for block from all peers if none provided

0805651

When requesting an explicit fork sync, try to sync from all known peers, when no specific peers were provided.

arkpar reviewed Oct 23, 2019

View reviewed changes

mxinden and others added 5 commits October 23, 2019 16:32

core/network/sync: Request specific fork sync from peers ahead or on par

6fda6a6

When making an explicit fork sync request without specifying any peers, make sure to only request it from the locally known peers that are either ahead or on a par compared to the block number we are looking for.

Merge remote-tracking branch 'paritytech/master' into sync_request_in…

2c5422f

…_until_imported

grandpa: fix tests

411e101

grandpa: fix warnings

6d695d1

grandpa: add test for block sync request on until_imported

202bced

andresilva force-pushed the sync_request_in_until_imported branch from 6fe3171 to 202bced Compare October 23, 2019 16:08

grandpa: rename Environment field inner to client

284ad34

andresilva reviewed Oct 23, 2019

View reviewed changes

grandpa: fix minor nits

f030ec6

andresilva reviewed Oct 23, 2019

View reviewed changes

andresilva added 3 commits October 23, 2019 17:44

grandpa: minor nits in until_imported

10790e7

grandpa: copy docs for set_sync_fork_request

3705a2c

grandpa: remove stale TODO on UntilImported

d668931

andresilva approved these changes Oct 23, 2019

View reviewed changes

mxinden commented Oct 23, 2019

View reviewed changes

arkpar approved these changes Oct 24, 2019

View reviewed changes

andresilva merged commit 5bd69ac into paritytech:master Oct 24, 2019

core/finality-grandpa: Request block sync from network after import timeout #3800

core/finality-grandpa: Request block sync from network after import timeout #3800

Uh oh!

Conversation

mxinden commented Oct 11, 2019

Goal

2nd solution (Grandpa vote messages and UntilImported)

Pull request status

Uh oh!

rphmeier Oct 12, 2019

Choose a reason for hiding this comment

Uh oh!

mxinden Oct 16, 2019

Choose a reason for hiding this comment

Uh oh!

andresilva left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andresilva Oct 17, 2019

Choose a reason for hiding this comment

Uh oh!

andresilva commented Oct 17, 2019

Uh oh!

arkpar Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

andresilva Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

andresilva Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

andresilva Oct 23, 2019

Choose a reason for hiding this comment

Uh oh!

andresilva Oct 23, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andresilva left a comment

Choose a reason for hiding this comment

Uh oh!

mxinden left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

andresilva left a comment •

edited

Loading

andresilva Oct 23, 2019 •

edited

Loading