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

BEEFY: gossip finality proofs #13727

Merged
merged 6 commits into from
Mar 30, 2023

Conversation

acatangiu
Copy link
Contributor

@acatangiu acatangiu commented Mar 27, 2023

Adds gossiping BEEFY finality proofs to the BEEFY gossip protocol.

  • bumps protocol version since message format is changed,
  • gossip validator accepts finality proofs for rounds within current set_id with round_num >= current_best_beefy,
  • gossip validator verifies incoming finality proofs - including signatures against validator set keys,
  • finality proofs have their own topic,
  • voter imports gossip finality proofs using same logic as block-import or on-demand finality proofs,
  • after handling any vote, if finality threshold is reached, the voter gossips finality proof containing threshold validator signatures.

Fixes #13733

@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 Mar 27, 2023
@acatangiu acatangiu requested a review from andresilva March 27, 2023 17:57
@acatangiu acatangiu self-assigned this Mar 27, 2023
Gossip finality proof in _both_ cases of reaching finality threshold
through votes:
1. threshold reached through self vote,
2. threshold reached through incoming vote.
Copy link
Contributor

@svyatonik svyatonik left a comment

Choose a reason for hiding this comment

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

Looks good!

client/consensus/beefy/src/worker.rs Outdated Show resolved Hide resolved
justif = gossip_proofs.next() => {
if let Some(justif) = justif {
// Gossiped justifications have already been verified by `GossipValidator`.
if let Err(err) = self.triage_incoming_justif(justif) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Just checking - the justification is verified by GossipValidator and we know it is valid. But equivocations are detected here, by BeefyWorker. And unless I'm missing something, they're only detected when we handle incoming votes. I wonder if it is possible for validator to make two votes and stay unnoticed because the second vote will be gossiped using finality proof? I believe that even if I'm right in my assumption, it is very unlikely scenario (given enough validators set length), so maybe something for the future

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, for a validator set of size N, it's possible now for m voters to "undetectably equivocate" by sending a vote for mmr_root_2, then including another vote for mmr_root_1 in a proof alongside 2*N/3 other votes for mmr_root_1.

  1. Because the precondition of required 2*N/3 "good" votes before being able to "undetectably equivocate", it means m can be at most N/3 in "perfect" network conditions where coordination of who sees what gossipped votes is 100% controlled by dishonest validators.
  2. Even in these perfect conditions where m=N/3, best you could do is partition the network to get N/3 honest voting on mmr_root_1, another N/3 honest voting on mmr_root_2 and N/3 dishonest equivocating both => at least one validator will have to visibly equivocate to get over threshold.

Also, because BEEFY is based on GRANDPA, finality point.2 above is not possible, honest validators cannot be convinced to vote on anything other than what GRANDPA finalized.

Ergo, I believe this is not a problem/risk.

client/consensus/beefy/src/communication/gossip.rs Outdated Show resolved Hide resolved
client/consensus/beefy/src/communication/gossip.rs Outdated Show resolved Hide resolved
client/consensus/beefy/src/communication/gossip.rs Outdated Show resolved Hide resolved
client/consensus/beefy/src/worker.rs Outdated Show resolved Hide resolved
client/consensus/beefy/src/worker.rs Outdated Show resolved Hide resolved
client/consensus/beefy/src/communication/gossip.rs Outdated Show resolved Hide resolved
@acatangiu
Copy link
Contributor Author

@serban300, I've addressed all comments

@acatangiu acatangiu merged commit 025325d into paritytech:master Mar 30, 2023
@acatangiu acatangiu deleted the beefy-gossip-justifications branch March 30, 2023 14:23
pgherveou pushed a commit that referenced this pull request Apr 4, 2023
* sc-consensus-beefy: add justifications to gossip protocol

* sc-consensus-beefy: voter gossips finality proofs

* sc-consensus-beefy: add finality proof gossip test

* sc-consensus-beefy: always gossip finality proof

Gossip finality proof in _both_ cases of reaching finality threshold
through votes:
1. threshold reached through self vote,
2. threshold reached through incoming vote.

* address comments
gpestana pushed a commit that referenced this pull request Apr 23, 2023
* sc-consensus-beefy: add justifications to gossip protocol

* sc-consensus-beefy: voter gossips finality proofs

* sc-consensus-beefy: add finality proof gossip test

* sc-consensus-beefy: always gossip finality proof

Gossip finality proof in _both_ cases of reaching finality threshold
through votes:
1. threshold reached through self vote,
2. threshold reached through incoming vote.

* address comments
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* sc-consensus-beefy: add justifications to gossip protocol

* sc-consensus-beefy: voter gossips finality proofs

* sc-consensus-beefy: add finality proof gossip test

* sc-consensus-beefy: always gossip finality proof

Gossip finality proof in _both_ cases of reaching finality threshold
through votes:
1. threshold reached through self vote,
2. threshold reached through incoming vote.

* address comments
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

beefy: enhance gossip protocol with finality proofs
3 participants