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

BEEFY subscription fires multiple times for the same commitment#10684

Closed
Wizdave97 wants to merge 26 commits intoparitytech:masterfrom
Wizdave97:david/beefy-subscribe-justification-rpc
Closed

BEEFY subscription fires multiple times for the same commitment#10684
Wizdave97 wants to merge 26 commits intoparitytech:masterfrom
Wizdave97:david/beefy-subscribe-justification-rpc

Conversation

@Wizdave97
Copy link
Contributor

@Wizdave97 Wizdave97 commented Jan 17, 2022

@Wizdave97 Wizdave97 marked this pull request as ready for review January 17, 2022 19:58
@Wizdave97 Wizdave97 requested a review from acatangiu as a code owner January 17, 2022 19:58
seunlanlege
seunlanlege suggested changes Jan 17, 2022
View reviewed changes
@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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jan 24, 2022
acatangiu
acatangiu approved these changes Jan 24, 2022
View reviewed changes
Copy link
Contributor

@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.

The code looks good in terms of making sure that RPC subscription doesn't forward duplicate (or old) commitments.

It's not clear why there are duplicate commitments to begin with... should we at least log something when seeing these duplicate commitments so we can further investigate?

bkchr
bkchr suggested changes Jan 24, 2022
View reviewed changes
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.

The issue mentions to find out why commitments are send twice and not to just prevent sending them multiple times from the rpc layer.

@seunlanlege
Copy link
Contributor

seunlanlege commented Jan 25, 2022 •
edited
Loading

The issue mentions to find out why commitments are send twice and not to just prevent sending them multiple times from the rpc layer.

@bkchr

Because of the way networking works, peers could still be broadcasting votes for a round that has already concluded. The best we can do is dedupe the notifications sent to the rpc clients.

@Wizdave97 Wizdave97 force-pushed the david/beefy-subscribe-justification-rpc branch from 9334637 to d0846ed Compare January 25, 2022 15:47
seunlanlege
seunlanlege reviewed Jan 28, 2022
View reviewed changes
client/beefy/rpc/src/lib.rs
}

fn create_commitment(block_number: u32) -> BeefySignedCommitment<Block> {
fn create_commitment(block_number: u64) -> BeefySignedCommitment<Block> {
Copy link
Contributor

@seunlanlege seunlanlege Jan 28, 2022

Choose a reason for hiding this comment

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

block numbers are actually u32

Copy link
Contributor Author

@Wizdave97 Wizdave97 Jan 28, 2022

Choose a reason for hiding this comment

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

The susbtrate-test-runtime uses a u64

@stale
Copy link

stale bot commented Feb 27, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Feb 27, 2022
@Wizdave97
Copy link
Contributor Author

Wizdave97 commented Feb 27, 2022

Bump

@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Feb 27, 2022
@acatangiu
Copy link
Contributor

acatangiu commented Mar 28, 2022

Fixed in #10882

@acatangiu acatangiu closed this Mar 28, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

3 more reviewers

@seunlanlege seunlanlege seunlanlege requested changes

@bkchr bkchr bkchr requested changes

@acatangiu acatangiu acatangiu approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

BEEFY subscription fires multiple times for the same commitment

4 participants

@Wizdave97 @seunlanlege @acatangiu @bkchr