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

Beefy on-demand justifications as a custom RequestResponse protocol #12124

Merged

Conversation

acatangiu
Copy link
Contributor

@acatangiu acatangiu commented Aug 26, 2022

Description

Run a 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.

PR defines BEEFY own RequestResponse sub-protocol and adds support for:

  • handling incoming requests: BeefyJustifsRequestHandler in incoming_requests_handler.rs
  • requesting justifications from peers: OnDemandJustificationsEngine in outgoing_requests_engine.rs

BeefyJustifsRequestHandler

Run as independent async task, accepts incoming requests, decodes and validates request for BEEFY justification for block b.
It then uses client backend to retrieve justification if available, encode and provide it as custom protocol response.

OnDemandJustificationsEngine

Simple async state machine that be either:

  • "idle" - no on-demand justifications required
  • "requesting justification for block b" - self driving future that sequentially sends requests to all relevant peers until one provides valid justification or no more peers available. relevant peers are those seen (through gossip) voting for block numbers >= b, meaning they should have justification for b.

main worker/voter

  1. Requests justifications from OnDemandJustificationsEngine for every mandatory block it's currently working/voting/blocked on.
  2. Drives the OnDemandJustificationsEngine inner future in its main loop.

Tests

Added integration test that verifies all components and full workflow:

  • 1 out of 4 voters is held behind so that it misses voting for a number of mandatory blocks,
  • when started, late voter must get justifications for past mandatory blocks and can only do so through on-demand requests,
  • other 3 voters need to provide on-demand justifications responses for late voter to catch up,
  • by end of test, late voter is up-to-date and actively participating in voting.

Fixes

Fixes #12093 (more details there).

polkadot companion: paritytech/polkadot#6035

@acatangiu acatangiu added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. labels Aug 26, 2022
@acatangiu acatangiu self-assigned this Aug 26, 2022
Fix corner case where voter gets a single burst of finality
notifications just when it starts.

The notification stream was consumed by "wait_for_pallet" logic,
then main loop would subscribe to finality notifications, but by that
time some notifications might've been lost.

Fix this by subscribing the main loop to notifications before waiting
for pallet to become available. Share the same stream with the main loop
so that notifications for blocks before pallet available are ignored,
while _all_ notifications after pallet available are processed.

Add regression test for this.

Signed-off-by: acatangiu <adrian@parity.io>
Copy link
Contributor

@altonen altonen left a comment

Choose a reason for hiding this comment

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

Left some minor comments but looks good!

Copy link
Contributor Author

@acatangiu acatangiu left a comment

Choose a reason for hiding this comment

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

Thanks for the review! I've addressed all comments 👍

client/beefy/src/communication/mod.rs Outdated Show resolved Hide resolved
pub async fn next(&mut self) -> Option<BeefyVersionedFinalityProof<B>> {
let (peer, block, resp) = match &mut self.state {
State::Idle(pending) => {
let _ = pending.next().await;
Copy link
Contributor

Choose a reason for hiding this comment

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

A noobs async question: does this rely on next() being polled in a select! loop with other futures, otherwise we'll get stuck here forever?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This whole function is async (as seen in pub async fn signature). This means it cannot be (directly) called from a normal sync context - the compiler will complain (this is because async functions actually translates to sync functions returning impl Future - a pretty good article on this here).

.await does indeed mean "wait here until this is ready", but it can only be called inside an async function, meaning "wait here" is not "block here", but rather "do something else until this is ready, then come back here".
The way this happens under the hood is by turning this whole function into a state machine where each await is a transition from one state to another - a pretty good explanation here.

So to answer your question, no, this function does not rely on it being called in a select! loop, the only requirements on it are the ones enforced by the compiler: the function needs to be called from either another async context/fn or must be directly called by an "asynchronous runtime".

Copy link
Contributor

@dmitry-markin dmitry-markin Oct 3, 2022

Choose a reason for hiding this comment

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

@acatangiu thanks a lot for the detailed explanation!

@acatangiu
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Error: Statuses failed for b904741

@acatangiu
Copy link
Contributor Author

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot paritytech-processbot bot merged commit 9472af8 into paritytech:master Oct 3, 2022
Copy link
Member

@bkchr bkchr left a comment

Choose a reason for hiding this comment

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

I know that I'm very late to the party. I still left some comments on stuff that could be improved. Sorry!

@acatangiu
Copy link
Contributor Author

I know that I'm very late to the party. I still left some comments on stuff that could be improved. Sorry!

Better late than never 😛 thanks for the improvement ideas! Opened #12414 for them.

ordian added a commit that referenced this pull request Oct 5, 2022
* master: (42 commits)
  Adapt `pallet-contracts` to WeightV2 (#12421)
  Improved election pallet testing (#12327)
  Bump prost to 0.11+ (#12419)
  Use saturating add for alliance::disband witness data (#12418)
  [Fix] Rename VoterBagsList -> VoterList to match pdot (#12416)
  client/beefy: small code improvements (#12414)
  BEEFY: Simplify hashing for pallet-beefy-mmr (#12393)
  Add @koute to `docs/CODEOWNERS` and update stale paths (#12408)
  docs/CODEOWNERS: add @acatangiu as MMR owner (#12406)
  Remove unnecessary Clone trait bounds on CountedStorageMap (#12402)
  Fix `Weight::is_zero` (#12396)
  Beefy on-demand justifications as a custom RequestResponse protocol (#12124)
  Remove contracts RPCs (#12358)
  pallet-mmr: generate historical proofs (#12324)
  unsafe_pruning flag removed (#12385)
  Carry over where clauses defined in Config to Call and Hook (#12388)
  Properly set the max proof size weight on defaults and tests (#12383)
  BEEFY: impl TypeInfo for SignedCommitment (#12382)
  bounding staking: `BoundedElectionProvider` trait (#12362)
  New Pallet: Root offences (#11943)
  ...
@acatangiu acatangiu deleted the beefy-custom-sync-protocol branch December 13, 2022 08:39
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
…aritytech#12124)

* client/beefy: create communication module and move gossip there

* client/beefy: move beefy_protocol_name module to communication

* client/beefy: move notification module under communication

* client/beefy: add incoming request_response protocol handler

* client/beefy: keep track of connected peers and their progress

* client/beefy: add logic for generating Justif requests

* client/beefy: cancel outdated on-demand justification requests

* try Andre's suggestion for JustificationEngine

* justif engine add justifs validation

* client/beefy: impl OnDemandJustificationsEngine async next()

* move beefy proto name test

* client/beefy: initialize OnDemandJustificationsEngine

* client/tests: allow for custom req-resp protocols

* client/beefy: on-demand-justif: implement simple peer selection strategy

* client/beefy: fix voter initialization

Fix corner case where voter gets a single burst of finality
notifications just when it starts.

The notification stream was consumed by "wait_for_pallet" logic,
then main loop would subscribe to finality notifications, but by that
time some notifications might've been lost.

Fix this by subscribing the main loop to notifications before waiting
for pallet to become available. Share the same stream with the main loop
so that notifications for blocks before pallet available are ignored,
while _all_ notifications after pallet available are processed.

Add regression test for this.

Signed-off-by: acatangiu <adrian@parity.io>

* client/beefy: make sure justif requests are always out for mandatory blocks

* client/beefy: add test for on-demand justifications sync

* client/beefy: tweak main loop event processing order

* client/beefy: run on-demand-justif-handler under same async task as voter

* client/beefy: add test for known-peers

* client/beefy: reorg request-response module

* client/beefy: add issue references for future work todos

* client/beefy: consolidate on-demand-justifications engine state machine

Signed-off-by: acatangiu <adrian@parity.io>

* client/beefy: fix for polkadot companion

* client/beefy: implement review suggestions

* cargo fmt and clippy

* fix merge damage

* fix rust-doc

* fix merge damage

* fix merge damage

* client/beefy: add test for justif proto name

Signed-off-by: acatangiu <adrian@parity.io>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D2-notlive 💤 PR contains changes in a runtime directory that is not deployed to a chain that requires an audit.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

BEEFY - justifications sync protocol
4 participants