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

BEEFY - justifications sync protocol #12093

Closed
Tracked by #1632
acatangiu opened this issue Aug 23, 2022 · 7 comments · Fixed by #12124
Closed
Tracked by #1632

BEEFY - justifications sync protocol #12093

acatangiu opened this issue Aug 23, 2022 · 7 comments · Fixed by #12124
Assignees
Labels
J0-enhancement An additional feature request. U1-asap No need to stop dead in your tracks, however issue should be addressed as soon as possible. Z4-involved Can be fixed by an expert coder with good knowledge of the codebase.

Comments

@acatangiu
Copy link
Contributor

This issue replaces paritytech/grandpa-bridge-gadget#20 and tracks implementing #3 below:

A node running BEEFY worker/gadget, generates/imports and stores BEEFY justifications as follows:

  1. when synced, it participates in voting gossip and builds BEEFY justifications itself from gossiped votes,
  2. when behind, it imports BEEFY justifications alongside imported blocks (as part of block import - added in Lean BEEFY to Full BEEFY - don't skip (older) mandatory blocks and import justifications #11821 and beefy: initialize voter from genesis and fix initial sync #11959),
  3. runs dedicated BEEFY justifications sync protocol as a libp2p RequestResponse protocol - where node can request BEEFY justifications for certain blocks from connected peers, and also answer any requests it gets.

#3 above is required when syncing from a peer that might be missing some/all BEEFY justifications.

@acatangiu acatangiu added J0-enhancement An additional feature request. U1-asap No need to stop dead in your tracks, however issue should be addressed as soon as possible. Z4-involved Can be fixed by an expert coder with good knowledge of the codebase. labels Aug 23, 2022
@acatangiu acatangiu self-assigned this Aug 23, 2022
@acatangiu acatangiu added this to BEEFY Aug 23, 2022
@acatangiu acatangiu moved this to In Progress 🛠 in BEEFY Aug 23, 2022
@acatangiu
Copy link
Contributor Author

I'm seeing all current RequestResponse protocols in Substrate network are built around sync and are also kind of hardcoded, not really "pluggable".

Should we also "hardcode" BEEFY support (at most wrap it around an Option<>, just like warp sync)?

Should we add it to (Protocol as NetworkBehavior)::poll() just like the others, maybe even through the ChainSync trait member?

Since this would be a separate sync, we'll end up with mixed state (some block-sync-specific and some BEEFY-sync-specific) in struct ChainSync, struct PeerSync and struct PeerSyncState. Any concerns here?

In case we get justifs from other sources is there any way to cancel any pending request?

What kind of peer selection strategy should we use?

@andresilva @bkchr @tomaka ^

@bkchr
Copy link
Member

bkchr commented Aug 24, 2022

I'm seeing all current RequestResponse protocols in Substrate network are built around sync and are also kind of hardcoded, not really "pluggable".

If you take a look at Polkadot you see pluggable protocols and this is the way forward. We will not accept anymore hardcoded protocols if there is not any special reason for this.

Should we also "hardcode" BEEFY support (at most wrap it around an Option<>, just like warp sync)?

No please don't do this. It should be its own protocol, living in its own crate.

Also just no to the other stuff above 🙈

In case we get justifs from other sources is there any way to cancel any pending request?

I don't think there is a way currently. What would be other sources to receive a justification?

What kind of peer selection strategy should we use?

I think for now we should be able to just follow the Event::SyncConnected/Event::SyncDisconnected:

/// Now connected to a new peer for syncing purposes.
SyncConnected {
/// Node we are now syncing from.
remote: PeerId,
},
/// Now disconnected from a peer for syncing purposes.
SyncDisconnected {
/// Node we are no longer syncing from.
remote: PeerId,
},

You can get these by getting an event_stream:

fn event_stream(&self, name: &'static str) -> Pin<Box<dyn Stream<Item = Event> + Send>> {

One problem is probably to know what blocks/justification a peer has. Maybe we should have our completely own protocol on top of sc-network that at least gossips to all peers when it has a certain justification imported?

@acatangiu
Copy link
Contributor Author

I don't think there is a way currently. What would be other sources to receive a justification?

  • Block import with attached justifications
  • Gossiped votes -> enough of them trigger a locally generated justification

I guess it's fine even without canceling a pending request, we'll just drop the obsolete response.

One problem is probably to know what blocks/justification a peer has. Maybe we should have our completely own protocol on top of sc-network that at least gossips to all peers when it has a certain justification imported?

We could use the votes gossiping to determine which peers have what (mandatory) justifications: any peer voting on block N will have all mandatory BEEFY justifications < N.
I'll also think about justification gossiping, see if we need that for any corner case.

If you take a look at Polkadot you see pluggable protocols and this is the way forward. We will not accept anymore hardcoded protocols if there is not any special reason for this.

Ok, got it. Will develop this as its own protocol, living in its own crate, "pluggable" into sc-network.

@bkchr
Copy link
Member

bkchr commented Aug 26, 2022

We could use the votes gossiping to determine which peers have what (mandatory) justifications: any peer voting on block N will have all mandatory BEEFY justifications < N.
I'll also think about justification gossiping, see if we need that for any corner case.

So all validators will have the justifications and only normal nodes will need to request them from validators? While doing major sync we send the BEEFY justification directly alongside the block? (Just trying to understand when we will need to request a justification)

@acatangiu
Copy link
Contributor Author

While doing major sync we send the BEEFY justification directly alongside the block?

yup, and the worker/voter gets them through BlockImport/JustificationImport.

Just trying to understand when we will need to request a justification

A validator was offline for a while and when coming back up syncs blocks from a node that also doesn't have (latest) BEEFY justifications => no justifs through block import, no justifs through voting because those rounds ended a while ago.
Worker/voter can then fallback to on-demand request missing mandatory justifications from peers.

@bkchr
Copy link
Member

bkchr commented Aug 26, 2022

A validator was offline for a while and when coming back up syncs blocks

Okay, so the validator comes back online. Syncs blocks without justfication. While doing this its already running the BEEFY protocol and listens for votes. Any node that votes on stuff with a higher block number than we are missing the justification for should have all justifications of all blocks before including the one we are searching for?

@acatangiu
Copy link
Contributor Author

Any node that votes on stuff with a higher block number than we are missing the justification for should have all justifications of all blocks before including the one we are searching for?

Yes, that’s right, any node voting for higher blocks should (if behaving correctly) have all mandatory block justifications leading to block number they’re currently voting on. And we’re only on-demand syncing mandatory ones so this approach should work.

@acatangiu acatangiu moved this to Draft in Parity Roadmap Sep 12, 2022
@acatangiu acatangiu moved this from Draft to Open in Parity Roadmap Sep 12, 2022
Repository owner moved this from In Progress 🛠 to Done ✅ in BEEFY Oct 3, 2022
Repository owner moved this from Open to Closed in Parity Roadmap Oct 3, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
J0-enhancement An additional feature request. U1-asap No need to stop dead in your tracks, however issue should be addressed as soon as possible. Z4-involved Can be fixed by an expert coder with good knowledge of the codebase.
Projects
No open projects
Status: Done
2 participants