Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

beginNextRound constains assumption on recieving local messages #351

Closed
Kubuxu opened this issue Jun 19, 2024 · 2 comments
Closed

beginNextRound constains assumption on recieving local messages #351

Kubuxu opened this issue Jun 19, 2024 · 2 comments
Assignees
@masih
Labels
bug Something isn't working gossipbft Relates to core GossipPBFT protocol
Milestone
Milestone 0: F3 A...

Comments

@Kubuxu
Copy link
Contributor

Kubuxu commented Jun 19, 2024 •
edited
Loading

go-f3/gpbft/gpbft.go

Lines 854 to 858 in ad199a8

// Extract the justification received from some participant (possibly this node itself).
justification, ok = prevRound.committed.receivedJustification[i.proposal.Key()]
if !ok {
panic("beginConverge called but no justification for proposal")
}

@adlrocha observes a panic from GPBFT in this piece of code.
Commit might require a similar treatment as converge: #334

Part of #319
@masih masih self-assigned this Jun 19, 2024
@masih masih added the gossipbft Relates to core GossipPBFT protocol label Jun 19, 2024
masih added a commit that referenced this issue Jun 19, 2024
@masih
Relax the need for synchronous broadcast of COMMIT messages to self
d92f597 
The message broadcast mechanism cannot guarantee that broadcasts to self
are delivered synchronously. This means it is possible for next round
with justification for non-bottom value from self to begin before the
justification for it is delivered.

Relax this assumption by directly updating the committed quorum state
when the participant itself find a strong quorum of PREPARE and builds a
justification for it. This change guarantees that regardless of the
order of message delivery in broadcast the next round can begin when
initiated by a justification from self.

Fixes #351
@masih masih mentioned this issue Jun 19, 2024
Closed
masih added a commit that referenced this issue Jun 19, 2024
@masih
Relax the need for synchronous broadcast of COMMIT messages to self
a4a2eda 
The message broadcast mechanism cannot guarantee that broadcasts to self
are delivered synchronously. This means it is possible for next round
with justification for non-bottom value from self to begin before the
justification for it is delivered.

Relax this assumption by directly updating the committed quorum state
when the participant itself find a strong quorum of PREPARE and builds a
justification for it. This change guarantees that regardless of the
order of message delivery in broadcast the next round can begin when
initiated by a justification from self.

Fixes #351
masih added a commit that referenced this issue Jun 19, 2024
@masih
Relax the need for synchronous broadcast of COMMIT messages to self
40d7962 
The message broadcast mechanism cannot guarantee that broadcasts to self
are delivered synchronously. This means it is possible for next round
with justification for non-bottom value from self to begin before the
justification for it is delivered.

Relax this assumption by directly updating the committed quorum state
when the participant itself find a strong quorum of PREPARE and builds a
justification for it. This change guarantees that regardless of the
order of message delivery in broadcast the next round can begin when
initiated by a justification from self.

Fixes #351
@anorth
Copy link
Member

anorth commented Jun 20, 2024 •
edited
Loading

I don't think this one is an assumption on receiving local messages because I don't think we can reach beginNextRound without receiving some message.

(In another channel, I learned that @adlrocha subsequently realised his panic was caused by another bug)

@anorth anorth added the bug Something isn't working label Jun 20, 2024
@masih
Copy link
Member

masih commented Jun 20, 2024 •
edited
Loading

I don't think this one is an assumption on receiving local messages because I don't think we can reach beginNextRound without receiving some message.

You are right. Thanks Alex.

The bug that caused this in @adlrocha's branch was related to missing power table values unrelated to the core protocol. I think we can close this issue. I'll close my PR.

@Kubuxu Kubuxu closed this as completed Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@masih masih

Labels
bug Something isn't working gossipbft Relates to core GossipPBFT protocol
Projects
F3
Status: Done
Milestone
Milestone 0: F3 Alpha Code Freeze
Development

Successfully merging a pull request may close this issue.

Relax the need for synchronous broadcast of COMMIT messages to self filecoin-project/go-f3
3 participants
@masih @anorth @Kubuxu