-
Notifications
You must be signed in to change notification settings - Fork 165
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.
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
FIP-0086 Audit final changes #996
FIP-0086 Audit final changes #996
Conversation
100 epochs including baseChain's head
@Kubuxu @masih @Stebalien just signalling this to you; covers the noted items in filecoin-project/go-f3#177 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good to me (just note the outstanding question from @anorth). Should we also include the proofs pdf in this PR?
@@ -760,7 +775,7 @@ The modifications proposed in this FIP have far-reaching implications for the se | |||
|
|||
## Incentive Considerations | |||
|
|||
Participating in GossiPBFT only entails verifying $O(n)$ and generating $O(1)$ signatures per participant. If not enough participants follow the protocol, the liveness of F3 will be affected. This means that the service offered is affected, but also that participants do not receive rewards from block proposals for the period in which they do not participate. Consequently, we do not believe additional incentives for participation are necessary, as the modifications in this FIP significantly improve the system and the additional computational and communication costs do not substantially alter the cost structure of running a Filecoin node. | |||
Participating in GossiPBFT only entails verifying $O(n)$ and generating $O(1)$ signatures per participant. If not enough participants follow the protocol, the liveness of F3 will be affected. This means that the service offered is affected, but also that participants do not receive rewards from block proposals for the period in which they do not participate, until the legacy finality of 900 epochs applies. Consequently, we do not believe additional incentives for participation are necessary, as the modifications in this FIP significantly improve the system and the additional computational and communication costs do not substantially alter the cost structure of running a Filecoin node. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if this phrasing is super clear -- probability better than the previous one. I also don't have great suggestions, alas.
Did I not already addressed that comment (you mean this one I guess?) (I did not accept the commit suggestion, but made it in a new commit myself, as I was considering other changes).
We can. I have just accepted suggestions on the two docs that are IMO at their final versions (GossiPBFT Implementation v2, and GossiPBFT message rebroadcast v3, with the former referencing the later in a new section). Lmk if you would like me to do it or if you are waiting on Alex North or others to agree to this before doing it yourself (or have me do it). 🙂 |
Co-authored-by: Jorge M. Soares <547492+jsoares@users.noreply.github.com>
You hadn't when I started the review -- you had by the time I submitted it. I just resolved the discussion, no further action needed. Let's sync in slack re pdfs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Issues in the previous review fixed, supplementary material added under resources.
* Change delta backoff parameter to 2.0. Specified in filecoin-project/FIPs#996. * Adjust comments about relationship between power table and base chain.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry I missed these last comments on previous reviews. I was expecting someone like @Stebalien or @Kubuxu to review that part
FIPS/fip-0086.md
Outdated
else: | ||
participants ← PowerTable(finalizedTipsets[0].tipset) // use finalizedTipsets[0]'s table the first 10 instances. | ||
proposal ← EC.HeaviestUnfinalizedChain() // get heaviest chain from EC | ||
if proposal.Head().epoch < EC.CurrentEpoch() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't this always the case? How can the proposal be for a future epoch?
participants ← PowerTable(finalizedTipsets[0].tipset) // use finalizedTipsets[0]'s table the first 10 instances. | ||
proposal ← EC.HeaviestUnfinalizedChain() // get heaviest chain from EC | ||
if proposal.Head().epoch < EC.CurrentEpoch() | ||
proposal = proposal[-1] // remove head from current epoch, as pre-agreement is unlikely |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does proposal[-1]
do? I would have guessed it selects the last tipset from the chain, but the comment seems to suggest it selects the opposite.
Addressing design issues listed as ii. (partially) iii. iv. and vi. from go-f3's #177.